From 18cbbc5052cd5a46caba7c0e8977929d35a3bbed Mon Sep 17 00:00:00 2001 From: Michael Schuster Date: Thu, 25 Jun 2020 16:52:40 +0200 Subject: [PATCH] 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 --- src/libsync/creds/keychainchunk.cpp | 112 +++++++++++++++++++++++++--- src/libsync/creds/keychainchunk.h | 38 ++++++++++ 2 files changed, 138 insertions(+), 12 deletions(-) diff --git a/src/libsync/creds/keychainchunk.cpp b/src/libsync/creds/keychainchunk.cpp index 836be0b01..69fcee12e 100644 --- a/src/libsync/creds/keychainchunk.cpp +++ b/src/libsync/creds/keychainchunk.cpp @@ -19,6 +19,8 @@ #include "configfile.h" #include "creds/abstractcredentials.h" +#include + 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(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 d1d207b55..772f69d63 100644 --- 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);