KeychainChunk: Add synchronous method startAwait()

Awaits completion with no need to connect some slot to the finished() signal first,
inspired by the ProxyAuthHandler class.

Also add:
- Job dtor to safely erase passwords
- textData() method
- New ctor overloads to work with arbitrary keys (without Account ptrs)

Signed-off-by: Michael Schuster <michael@schuster.ms>

diff --git a/src/libsync/creds/keychainchunk.cpp b/src/libsync/creds/keychainchunk.cpp
index 836be0b01b264c3295f98c7b7b6e347e0681a965..69fcee12e59707186d74cc604b4976a9798d977e 100644 (file)
--- a/src/libsync/creds/keychainchunk.cpp
+++ b/src/libsync/creds/keychainchunk.cpp
@@ -19,6 +19,8 @@
 #include "configfile.h"
 #include "creds/abstractcredentials.h"
 
+#include <QApplication>
+
 using namespace QKeychain;
 
 namespace OCC {
@@ -46,6 +48,12 @@ Job::Job(QObject *parent)
     _serviceName = Theme::instance()->appName();
 }
 
+Job::~Job()
+{
+    _chunkCount = 0;
+    _chunkBuffer.clear();
+}
+
 /*
 * WriteJob
 */
@@ -61,11 +69,45 @@ WriteJob::WriteJob(Account *account, const QString &key, const QByteArray &data,
     _chunkCount = 0;
 }
 
+WriteJob::WriteJob(const QString &key, const QByteArray &data, QObject *parent)
+    : WriteJob(nullptr, key, data, parent)
+{
+#ifdef Q_OS_WIN
+    // NOTE: The following is normally done in AbstractCredentials::keychainKey
+    //       when an _account is specified by our other ctr overload (see 'kck' in this file).
+
+    // On Windows the credential keys aren't namespaced properly
+    // by qtkeychain. To work around that we manually add namespacing
+    // to the generated keys. See #6125.
+    // It's safe to do that since the key format is changing for 2.4
+    // anyway to include the account ids. That means old keys can be
+    // migrated to new namespaced keys on windows for 2.4.
+    _key.prepend(QCoreApplication::applicationName() + "_");
+#endif
+}
+
 void WriteJob::start()
 {
+    _isJobRunning = true;
     slotWriteJobDone(nullptr);
 }
 
+bool WriteJob::startAwait()
+{
+    start();
+
+    while (_isJobRunning) {
+        QApplication::processEvents(QEventLoop::AllEvents, 200);
+    }
+
+    if (error() != NoError) {
+        qCWarning(lcKeychainChunk) << "WritePasswordJob failed with" << errorString();
+        return false;
+    }
+
+    return true;
+}
+
 void WriteJob::slotWriteJobDone(QKeychain::Job *incomingJob)
 {
     auto *writeJob = static_cast<QKeychain::WritePasswordJob *>(incomingJob);
@@ -105,14 +147,17 @@ void WriteJob::slotWriteJobDone(QKeychain::Job *incomingJob)
 
             _chunkBuffer.clear();
 
+            _isJobRunning = false;
             emit finished(this);
             return;
         }
 
-        const QString kck = AbstractCredentials::keychainKey(
-            _account->url().toString(),
-            _key + (index > 0 ? (QString(".") + QString::number(index)) : QString()),
-            _account->id());
+        const QString keyWithIndex = _key + (index > 0 ? (QString(".") + QString::number(index)) : QString());
+        const QString kck = _account ? AbstractCredentials::keychainKey(
+                _account->url().toString(),
+                keyWithIndex,
+                _account->id()
+            ) : keyWithIndex;
 
         auto *job = new QKeychain::WritePasswordJob(_serviceName);
 #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK)
@@ -127,6 +172,7 @@ void WriteJob::slotWriteJobDone(QKeychain::Job *incomingJob)
 
         chunk.clear();
     } else {
+        _isJobRunning = false;
         emit finished(this);
     }
 
@@ -148,15 +194,33 @@ ReadJob::ReadJob(Account *account, const QString &key, const bool &keychainMigra
     _chunkBuffer.clear();
 }
 
+ReadJob::ReadJob(const QString &key, QObject *parent)
+    : ReadJob(nullptr, key, false, parent)
+{
+#ifdef Q_OS_WIN
+    // NOTE: The following is normally done in AbstractCredentials::keychainKey
+    //       when an _account is specified by our other ctr overload (see 'kck' in this file).
+
+    // On Windows the credential keys aren't namespaced properly
+    // by qtkeychain. To work around that we manually add namespacing
+    // to the generated keys. See #6125.
+    // It's safe to do that since the key format is changing for 2.4
+    // anyway to include the account ids. That means old keys can be
+    // migrated to new namespaced keys on windows for 2.4.
+    _key.prepend(QCoreApplication::applicationName() + "_");
+#endif
+}
+
 void ReadJob::start()
 {
     _chunkCount = 0;
     _chunkBuffer.clear();
 
-    const QString kck = AbstractCredentials::keychainKey(
-        _account->url().toString(),
-        _key,
-        _keychainMigration ? QString() : _account->id());
+    const QString kck = _account ? AbstractCredentials::keychainKey(
+            _account->url().toString(),
+            _key,
+            _keychainMigration ? QString() : _account->id()
+        ) : _key;
 
     auto *job = new QKeychain::ReadPasswordJob(_serviceName);
 #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK)
@@ -165,9 +229,30 @@ void ReadJob::start()
     job->setInsecureFallback(_insecureFallback);
     job->setKey(kck);
     connect(job, &QKeychain::Job::finished, this, &KeychainChunk::ReadJob::slotReadJobDone);
+    _isJobRunning = true;
     job->start();
 }
 
+bool ReadJob::startAwait()
+{
+    start();
+
+    while (_isJobRunning) {
+        QApplication::processEvents(QEventLoop::AllEvents, 200);
+    }
+
+    if (error() == NoError) {
+        return true;
+    }
+
+    _chunkCount = 0;
+    _chunkBuffer.clear();
+    if (error() != EntryNotFound) {
+        qCWarning(lcKeychainChunk) << "ReadPasswordJob failed with" << errorString();
+    }
+    return false;
+}
+
 void ReadJob::slotReadJobDone(QKeychain::Job *incomingJob)
 {
     // Errors or next chunk?
@@ -181,10 +266,12 @@ void ReadJob::slotReadJobDone(QKeychain::Job *incomingJob)
 #if defined(Q_OS_WIN)
             // try to fetch next chunk
             if (_chunkCount < KeychainChunk::MaxChunks) {
-                const QString kck = AbstractCredentials::keychainKey(
-                    _account->url().toString(),
-                    _key + QString(".") + QString::number(_chunkCount),
-                    _keychainMigration ? QString() : _account->id());
+                const QString keyWithIndex = _key + QString(".") + QString::number(_chunkCount);
+                const QString kck = _account ? AbstractCredentials::keychainKey(
+                        _account->url().toString(),
+                        keyWithIndex,
+                        _keychainMigration ? QString() : _account->id()
+                    ) : keyWithIndex;
 
                 QKeychain::ReadPasswordJob *job = new QKeychain::ReadPasswordJob(_serviceName);
 #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK)
@@ -230,6 +317,7 @@ void ReadJob::slotReadJobDone(QKeychain::Job *incomingJob)
         readJob->deleteLater();
     }
 
+    _isJobRunning = false;
     emit finished(this);
 }
 

diff --git a/src/libsync/creds/keychainchunk.h b/src/libsync/creds/keychainchunk.h
index d1d207b5578ce08c0aa2e468fcf933f2f696ff32..772f69d63cd137d100b6325aab26720ccfbbe8e4 100644 (file)
--- a/src/libsync/creds/keychainchunk.h
+++ b/src/libsync/creds/keychainchunk.h
@@ -45,6 +45,8 @@ class Job : public QObject
 public:
     Job(QObject *parent = nullptr);
 
+    virtual ~Job();
+
     const QKeychain::Error error() const {
         return _error;
     }
@@ -55,6 +57,9 @@ public:
     QByteArray binaryData() const {
         return _chunkBuffer;
     }
+    QString textData() const {
+        return _chunkBuffer;
+    }
 
     const bool insecureFallback() const {
         return _insecureFallback;
@@ -74,6 +79,7 @@ protected:
     QString _key;
     bool _insecureFallback = false;
     bool _keychainMigration = false;
+    bool _isJobRunning = false;
 
     QKeychain::Error _error = QKeychain::NoError;
     QString _errorString;
@@ -90,8 +96,24 @@ class OWNCLOUDSYNC_EXPORT WriteJob : public KeychainChunk::Job
     Q_OBJECT
 public:
     WriteJob(Account *account, const QString &key, const QByteArray &data, QObject *parent = nullptr);
+    WriteJob(const QString &key, const QByteArray &data, QObject *parent = nullptr);
+
+    /**
+     * Call this method to start the job (async).
+     * You should connect some slot to the finished() signal first.
+     *
+     * @see QKeychain::Job::start()
+     */
     void start();
 
+    /**
+     * Call this method to start the job synchronously.
+     * Awaits completion with no need to connect some slot to the finished() signal first.
+     *
+     * @return Returns true on succeess (QKeychain::NoError).
+    */
+    bool startAwait();
+
 signals:
     void finished(KeychainChunk::WriteJob *incomingJob);
 
@@ -107,8 +129,24 @@ class OWNCLOUDSYNC_EXPORT ReadJob : public KeychainChunk::Job
     Q_OBJECT
 public:
     ReadJob(Account *account, const QString &key, const bool &keychainMigration, QObject *parent = nullptr);
+    ReadJob(const QString &key, QObject *parent = nullptr);
+
+    /**
+     * Call this method to start the job (async).
+     * You should connect some slot to the finished() signal first.
+     *
+     * @see QKeychain::Job::start()
+     */
     void start();
 
+    /**
+     * Call this method to start the job synchronously.
+     * Awaits completion with no need to connect some slot to the finished() signal first.
+     *
+     * @return Returns true on succeess (QKeychain::NoError).
+    */
+    bool startAwait();
+
 signals:
     void finished(KeychainChunk::ReadJob *incomingJob);