From 203a2ce0031046f454d4120f53ad437d33763eaa Mon Sep 17 00:00:00 2001 From: Michael Schuster Date: Thu, 25 Jun 2020 00:30:41 +0200 Subject: [PATCH 1/9] Move QKeychain::NoBackendAvailable error handling to KeychainChunk class Originally this was in the WebFlowCredentials class. Since we've abstracted everything from there already, let's also move this in case some other code may use KeychainChunk::ReadJob prior to WebFlowCredentials. Signed-off-by: Michael Schuster --- src/gui/creds/keychainchunk.cpp | 21 +++++++++++++++++++-- src/gui/creds/keychainchunk.h | 5 +++++ src/gui/creds/webflowcredentials.cpp | 15 --------------- src/gui/creds/webflowcredentials.h | 1 - 4 files changed, 24 insertions(+), 18 deletions(-) diff --git a/src/gui/creds/keychainchunk.cpp b/src/gui/creds/keychainchunk.cpp index cd6d3257f..836be0b01 100644 --- a/src/gui/creds/keychainchunk.cpp +++ b/src/gui/creds/keychainchunk.cpp @@ -202,8 +202,25 @@ void ReadJob::slotReadJobDone(QKeychain::Job *incomingJob) } #endif } else { - if (readJob->error() != QKeychain::Error::EntryNotFound || - ((readJob->error() == QKeychain::Error::EntryNotFound) && _chunkCount == 0)) { +#if defined(Q_OS_UNIX) && !defined(Q_OS_MAC) + if (!readJob->insecureFallback()) { // If insecureFallback is set, the next test would be pointless + if (_retryOnKeyChainError && (readJob->error() == QKeychain::NoBackendAvailable + || readJob->error() == QKeychain::OtherError)) { + // Could be that the backend was not yet available. Wait some extra seconds. + // (Issues #4274 and #6522) + // (For kwallet, the error is OtherError instead of NoBackendAvailable, maybe a bug in QtKeychain) + qCInfo(lcKeychainChunk) << "Backend unavailable (yet?) Retrying in a few seconds." << readJob->errorString(); + QTimer::singleShot(10000, this, &ReadJob::start); + _retryOnKeyChainError = false; + readJob->deleteLater(); + return; + } + _retryOnKeyChainError = false; + } +#endif + + if (readJob->error() != QKeychain::EntryNotFound || + ((readJob->error() == QKeychain::EntryNotFound) && _chunkCount == 0)) { _error = readJob->error(); _errorString = readJob->errorString(); qCWarning(lcKeychainChunk) << "Unable to read" << readJob->key() << "chunk" << QString::number(_chunkCount) << readJob->errorString(); diff --git a/src/gui/creds/keychainchunk.h b/src/gui/creds/keychainchunk.h index 875ab5037..d1ae1e9dd 100644 --- a/src/gui/creds/keychainchunk.h +++ b/src/gui/creds/keychainchunk.h @@ -111,6 +111,11 @@ signals: private slots: void slotReadJobDone(QKeychain::Job *incomingJob); + +#if defined(Q_OS_UNIX) && !defined(Q_OS_MAC) +private: + bool _retryOnKeyChainError = true; // true if we haven't done yet any reading from keychain +#endif }; // class ReadJob } // namespace KeychainChunk diff --git a/src/gui/creds/webflowcredentials.cpp b/src/gui/creds/webflowcredentials.cpp index ba762ee0c..ff5c2eb2a 100644 --- a/src/gui/creds/webflowcredentials.cpp +++ b/src/gui/creds/webflowcredentials.cpp @@ -450,21 +450,6 @@ void WebFlowCredentials::fetchFromKeychainHelper() { void WebFlowCredentials::slotReadClientCertPEMJobDone(KeychainChunk::ReadJob *readJob) { -#if defined(Q_OS_UNIX) && !defined(Q_OS_MAC) - Q_ASSERT(!readJob->insecureFallback()); // If insecureFallback is set, the next test would be pointless - if (_retryOnKeyChainError && (readJob->error() == QKeychain::NoBackendAvailable - || readJob->error() == QKeychain::OtherError)) { - // Could be that the backend was not yet available. Wait some extra seconds. - // (Issues #4274 and #6522) - // (For kwallet, the error is OtherError instead of NoBackendAvailable, maybe a bug in QtKeychain) - qCInfo(lcWebFlowCredentials) << "Backend unavailable (yet?) Retrying in a few seconds." << readJob->errorString(); - QTimer::singleShot(10000, this, &WebFlowCredentials::fetchFromKeychainHelper); - _retryOnKeyChainError = false; - return; - } - _retryOnKeyChainError = false; -#endif - // Store PEM in memory if (readJob->error() == NoError && readJob->binaryData().length() > 0) { QList sslCertificateList = QSslCertificate::fromData(readJob->binaryData(), QSsl::Pem); diff --git a/src/gui/creds/webflowcredentials.h b/src/gui/creds/webflowcredentials.h index 511ab542e..3f9cee38d 100644 --- a/src/gui/creds/webflowcredentials.h +++ b/src/gui/creds/webflowcredentials.h @@ -122,7 +122,6 @@ protected: bool _ready = false; bool _credentialsValid = false; bool _keychainMigration = false; - bool _retryOnKeyChainError = true; // true if we haven't done yet any reading from keychain WebFlowCredentialsDialog *_askDialog = nullptr; }; From 2a3ef044be79b90f479ea24e49bbd3ebf48583e5 Mon Sep 17 00:00:00 2001 From: Michael Schuster Date: Thu, 25 Jun 2020 01:08:59 +0200 Subject: [PATCH 2/9] Move KeychainChunk class from gui to libsync Signed-off-by: Michael Schuster --- src/gui/CMakeLists.txt | 1 - src/gui/creds/webflowcredentials.cpp | 3 +-- src/libsync/CMakeLists.txt | 1 + src/{gui => libsync}/creds/keychainchunk.cpp | 0 src/{gui => libsync}/creds/keychainchunk.h | 9 ++++++--- 5 files changed, 8 insertions(+), 6 deletions(-) rename src/{gui => libsync}/creds/keychainchunk.cpp (100%) rename src/{gui => libsync}/creds/keychainchunk.h (95%) diff --git a/src/gui/CMakeLists.txt b/src/gui/CMakeLists.txt index b83106441..57c876c33 100644 --- a/src/gui/CMakeLists.txt +++ b/src/gui/CMakeLists.txt @@ -112,7 +112,6 @@ set(client_SRCS creds/httpcredentialsgui.cpp creds/oauth.cpp creds/flow2auth.cpp - creds/keychainchunk.cpp creds/webflowcredentials.cpp creds/webflowcredentialsdialog.cpp wizard/postfixlineedit.cpp diff --git a/src/gui/creds/webflowcredentials.cpp b/src/gui/creds/webflowcredentials.cpp index ff5c2eb2a..e6c1f141a 100644 --- a/src/gui/creds/webflowcredentials.cpp +++ b/src/gui/creds/webflowcredentials.cpp @@ -1,13 +1,13 @@ #include "webflowcredentials.h" #include "creds/httpcredentials.h" +#include "creds/keychainchunk.h" #include #include #include #include #include -#include #include #include #include @@ -18,7 +18,6 @@ #include "theme.h" #include "wizard/webview.h" #include "webflowcredentialsdialog.h" -#include "keychainchunk.h" using namespace QKeychain; diff --git a/src/libsync/CMakeLists.txt b/src/libsync/CMakeLists.txt index 146e0910a..aef76d246 100644 --- a/src/libsync/CMakeLists.txt +++ b/src/libsync/CMakeLists.txt @@ -59,6 +59,7 @@ set(libsync_SRCS creds/dummycredentials.cpp creds/abstractcredentials.cpp creds/credentialscommon.cpp + creds/keychainchunk.cpp ) if(TOKEN_AUTH_ONLY) diff --git a/src/gui/creds/keychainchunk.cpp b/src/libsync/creds/keychainchunk.cpp similarity index 100% rename from src/gui/creds/keychainchunk.cpp rename to src/libsync/creds/keychainchunk.cpp diff --git a/src/gui/creds/keychainchunk.h b/src/libsync/creds/keychainchunk.h similarity index 95% rename from src/gui/creds/keychainchunk.h rename to src/libsync/creds/keychainchunk.h index d1ae1e9dd..d1d207b55 100644 --- a/src/gui/creds/keychainchunk.h +++ b/src/libsync/creds/keychainchunk.h @@ -39,7 +39,8 @@ static constexpr int MaxChunks = 10; /* * @brief: Abstract base class for KeychainChunk jobs. */ -class Job : public QObject { +class Job : public QObject +{ Q_OBJECT public: Job(QObject *parent = nullptr); @@ -84,7 +85,8 @@ protected: /* * @brief: Simple wrapper class for QKeychain::WritePasswordJob, splits too large keychain entry's data into chunks on Windows */ -class WriteJob : public KeychainChunk::Job { +class OWNCLOUDSYNC_EXPORT WriteJob : public KeychainChunk::Job +{ Q_OBJECT public: WriteJob(Account *account, const QString &key, const QByteArray &data, QObject *parent = nullptr); @@ -100,7 +102,8 @@ private slots: /* * @brief: Simple wrapper class for QKeychain::ReadPasswordJob, splits too large keychain entry's data into chunks on Windows */ -class ReadJob : public KeychainChunk::Job { +class OWNCLOUDSYNC_EXPORT ReadJob : public KeychainChunk::Job +{ Q_OBJECT public: ReadJob(Account *account, const QString &key, const bool &keychainMigration, QObject *parent = nullptr); From 18cbbc5052cd5a46caba7c0e8977929d35a3bbed Mon Sep 17 00:00:00 2001 From: Michael Schuster Date: Thu, 25 Jun 2020 16:52:40 +0200 Subject: [PATCH 3/9] 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); From 81c644e7025829cfd973db618724e0751e1fa162 Mon Sep 17 00:00:00 2001 From: Michael Schuster Date: Thu, 25 Jun 2020 16:59:57 +0200 Subject: [PATCH 4/9] ConfigFile security: Migrate Proxy password to keychain When specified in the config file, the Proxy password will be migrated to the keychain, for backward compatibility and to allow admins to overwrite an existing password by rolling out updated config files. Once migrated to the keychain, the password will be removed from the config file. Signed-off-by: Michael Schuster --- src/gui/creds/webflowcredentials.cpp | 6 +--- src/libsync/configfile.cpp | 51 ++++++++++++++++++++++++++-- src/libsync/configfile.h | 2 ++ 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/src/gui/creds/webflowcredentials.cpp b/src/gui/creds/webflowcredentials.cpp index e6c1f141a..52eef0341 100644 --- a/src/gui/creds/webflowcredentials.cpp +++ b/src/gui/creds/webflowcredentials.cpp @@ -598,11 +598,7 @@ void WebFlowCredentials::deleteKeychainEntries(bool oldKeychainEntries) { job->setKey(keychainKey(_account->url().toString(), key, oldKeychainEntries ? QString() : _account->id())); - - connect(job, &Job::finished, this, [](QKeychain::Job *job) { - auto *djob = qobject_cast(job); - djob->deleteLater(); - }); + job->setAutoDelete(true); job->start(); }; diff --git a/src/libsync/configfile.cpp b/src/libsync/configfile.cpp index 39bd1d170..299ab09d1 100644 --- a/src/libsync/configfile.cpp +++ b/src/libsync/configfile.cpp @@ -20,6 +20,7 @@ #include "common/asserts.h" #include "creds/abstractcredentials.h" +#include "creds/keychainchunk.h" #include "csync_exclude.h" @@ -651,7 +652,23 @@ void ConfigFile::setProxyType(int proxyType, settings.setValue(QLatin1String(proxyPortC), port); settings.setValue(QLatin1String(proxyNeedsAuthC), needsAuth); settings.setValue(QLatin1String(proxyUserC), user); - settings.setValue(QLatin1String(proxyPassC), pass.toUtf8().toBase64()); + + if (pass.isEmpty()) { + settings.remove(QLatin1String(proxyPassC)); + + auto *job = new QKeychain::DeletePasswordJob(Theme::instance()->appName()); + job->setInsecureFallback(false); + job->setKey(keychainProxyPasswordKey()); + job->setAutoDelete(true); + job->start(); + } else { + // Write password to keychain + auto *job = new KeychainChunk::WriteJob(keychainProxyPasswordKey(), pass.toUtf8()); + if (job->startAwait()) { + settings.remove(QLatin1String(proxyPassC)); + } + job->deleteLater(); + } } settings.sync(); } @@ -726,8 +743,36 @@ QString ConfigFile::proxyUser() const QString ConfigFile::proxyPassword() const { - QByteArray pass = getValue(proxyPassC).toByteArray(); - return QString::fromUtf8(QByteArray::fromBase64(pass)); + QByteArray passEncoded = getValue(proxyPassC).toByteArray(); + auto pass = QString::fromUtf8(QByteArray::fromBase64(passEncoded)); + passEncoded.clear(); + + const auto key = keychainProxyPasswordKey(); + + if (!pass.isEmpty()) { + // Security: Migrate password from config file to keychain + auto *job = new KeychainChunk::WriteJob(key, pass.toUtf8()); + if (job->startAwait()) { + QSettings settings(configFile(), QSettings::IniFormat); + settings.remove(QLatin1String(proxyPassC)); + qCInfo(lcConfigFile()) << "Migrated proxy password to keychain for" << key; + } + job->deleteLater(); + } else { + // Read password from keychain + auto *job = new KeychainChunk::ReadJob(key); + if (job->startAwait()) { + pass = job->textData(); + } + job->deleteLater(); + } + + return pass; +} + +QString ConfigFile::keychainProxyPasswordKey() const +{ + return QString::fromLatin1("proxy-password"); } int ConfigFile::useUploadLimit() const diff --git a/src/libsync/configfile.h b/src/libsync/configfile.h index 413312375..432720a3a 100644 --- a/src/libsync/configfile.h +++ b/src/libsync/configfile.h @@ -198,6 +198,8 @@ private: const QVariant &defaultValue = QVariant()) const; void setValue(const QString &key, const QVariant &value); + QString keychainProxyPasswordKey() const; + private: typedef QSharedPointer SharedCreds; From ea95c4bf49437a9e964e192db3406107c614821c Mon Sep 17 00:00:00 2001 From: Michael Schuster Date: Sat, 27 Jun 2020 04:25:36 +0200 Subject: [PATCH 5/9] Refactor ProxyAuthHandler to use QEventLoop Signed-off-by: Michael Schuster --- src/gui/proxyauthhandler.cpp | 33 ++++++++++++--------------------- src/gui/proxyauthhandler.h | 2 -- 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/src/gui/proxyauthhandler.cpp b/src/gui/proxyauthhandler.cpp index 39310ccf4..010093b54 100644 --- a/src/gui/proxyauthhandler.cpp +++ b/src/gui/proxyauthhandler.cpp @@ -131,12 +131,7 @@ void ProxyAuthHandler::handleProxyAuthenticationRequired( this, &ProxyAuthHandler::slotSenderDestroyed); } } - -void ProxyAuthHandler::slotKeychainJobDone() -{ - _keychainJobRunning = false; -} - + void ProxyAuthHandler::slotSenderDestroyed(QObject *obj) { _gaveCredentialsTo.remove(obj); @@ -144,10 +139,13 @@ void ProxyAuthHandler::slotSenderDestroyed(QObject *obj) bool ProxyAuthHandler::getCredsFromDialog() { + QEventLoop waitLoop; + // Open the credentials dialog if (!_waitingForDialog) { _dialog->reset(); _dialog->setProxyAddress(_proxy); + connect(_dialog, &QDialog::finished, &waitLoop, &QEventLoop::quit); _dialog->open(); } @@ -155,8 +153,8 @@ bool ProxyAuthHandler::getCredsFromDialog() // If that's the case, continue processing the dialog until // it's done. ++_waitingForDialog; - while (_dialog && _dialog->isVisible()) { - QApplication::processEvents(QEventLoop::ExcludeSocketNotifiers, 200); + if (_dialog) { + waitLoop.exec(QEventLoop::ExcludeSocketNotifiers); } --_waitingForDialog; @@ -172,6 +170,7 @@ bool ProxyAuthHandler::getCredsFromDialog() bool ProxyAuthHandler::getCredsFromKeychain() { using namespace QKeychain; + QEventLoop waitLoop; if (_waitingForDialog) { return false; @@ -190,9 +189,7 @@ bool ProxyAuthHandler::getCredsFromKeychain() _readPasswordJob->setInsecureFallback(false); _readPasswordJob->setKey(keychainPasswordKey()); _readPasswordJob->setAutoDelete(false); - connect(_readPasswordJob.data(), &QKeychain::Job::finished, - this, &ProxyAuthHandler::slotKeychainJobDone); - _keychainJobRunning = true; + connect(_readPasswordJob.data(), &QKeychain::Job::finished, &waitLoop, &QEventLoop::quit); _readPasswordJob->start(); } @@ -201,10 +198,7 @@ bool ProxyAuthHandler::getCredsFromKeychain() // bad behavior when we reenter this code after the flag has been switched // but before the while loop has finished. ++_waitingForKeychain; - _keychainJobRunning = true; - while (_keychainJobRunning) { - QApplication::processEvents(QEventLoop::AllEvents, 200); - } + waitLoop.exec(); --_waitingForKeychain; if (_readPasswordJob->error() == NoError) { @@ -223,6 +217,7 @@ bool ProxyAuthHandler::getCredsFromKeychain() void ProxyAuthHandler::storeCredsInKeychain() { using namespace QKeychain; + QEventLoop waitLoop; if (_waitingForKeychain) { return; @@ -238,15 +233,11 @@ void ProxyAuthHandler::storeCredsInKeychain() job->setKey(keychainPasswordKey()); job->setTextData(_password); job->setAutoDelete(false); - connect(job, &QKeychain::Job::finished, this, &ProxyAuthHandler::slotKeychainJobDone); - _keychainJobRunning = true; + connect(job, &QKeychain::Job::finished, &waitLoop, &QEventLoop::quit); job->start(); ++_waitingForKeychain; - _keychainJobRunning = true; - while (_keychainJobRunning) { - QApplication::processEvents(QEventLoop::AllEvents, 200); - } + waitLoop.exec(); --_waitingForKeychain; job->deleteLater(); diff --git a/src/gui/proxyauthhandler.h b/src/gui/proxyauthhandler.h index 6e4e24902..ccc6d824b 100644 --- a/src/gui/proxyauthhandler.h +++ b/src/gui/proxyauthhandler.h @@ -57,7 +57,6 @@ public slots: QAuthenticator *authenticator); private slots: - void slotKeychainJobDone(); void slotSenderDestroyed(QObject *); private: @@ -91,7 +90,6 @@ private: /// waiting for. int _waitingForDialog = 0; int _waitingForKeychain = 0; - bool _keychainJobRunning = false; QPointer _dialog; From 42eb3388f8b4249c61549029aa0f0fe93c0c6aec Mon Sep 17 00:00:00 2001 From: Michael Schuster Date: Sat, 27 Jun 2020 04:48:57 +0200 Subject: [PATCH 6/9] Refactor KeychainChunk to use QEventLoop and add DeleteJob class - Use QEventLoop for synchronous exec() - Rename startAwait() to exec() - Add code for auto deletion - Add new DeleteJob class - Cleanup, tweaks Signed-off-by: Michael Schuster --- src/libsync/creds/keychainchunk.cpp | 189 ++++++++++++++++++++++------ src/libsync/creds/keychainchunk.h | 57 ++++++++- 2 files changed, 206 insertions(+), 40 deletions(-) diff --git a/src/libsync/creds/keychainchunk.cpp b/src/libsync/creds/keychainchunk.cpp index 69fcee12e..5dc0ff2a2 100644 --- a/src/libsync/creds/keychainchunk.cpp +++ b/src/libsync/creds/keychainchunk.cpp @@ -39,6 +39,22 @@ static void addSettingsToJob(Account *account, QKeychain::Job *job) } #endif +#ifdef Q_OS_WIN +void jobKeyPrependAppName(QString &key) +{ + // 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 + /* * Job */ @@ -73,32 +89,24 @@ 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() + "_"); + jobKeyPrependAppName(_key); #endif } void WriteJob::start() { - _isJobRunning = true; + _error = QKeychain::NoError; + slotWriteJobDone(nullptr); } -bool WriteJob::startAwait() +bool WriteJob::exec() { start(); - while (_isJobRunning) { - QApplication::processEvents(QEventLoop::AllEvents, 200); - } + QEventLoop waitLoop; + connect(this, &WriteJob::finished, &waitLoop, &QEventLoop::quit); + waitLoop.exec(); if (error() != NoError) { qCWarning(lcKeychainChunk) << "WritePasswordJob failed with" << errorString(); @@ -147,8 +155,10 @@ void WriteJob::slotWriteJobDone(QKeychain::Job *incomingJob) _chunkBuffer.clear(); - _isJobRunning = false; emit finished(this); + + if (_autoDelete) + deleteLater(); return; } @@ -159,7 +169,7 @@ void WriteJob::slotWriteJobDone(QKeychain::Job *incomingJob) _account->id() ) : keyWithIndex; - auto *job = new QKeychain::WritePasswordJob(_serviceName); + auto *job = new QKeychain::WritePasswordJob(_serviceName, this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif @@ -172,8 +182,10 @@ void WriteJob::slotWriteJobDone(QKeychain::Job *incomingJob) chunk.clear(); } else { - _isJobRunning = false; emit finished(this); + + if (_autoDelete) + deleteLater(); } writeJob->deleteLater(); @@ -198,16 +210,7 @@ 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() + "_"); + jobKeyPrependAppName(_key); #endif } @@ -215,6 +218,7 @@ void ReadJob::start() { _chunkCount = 0; _chunkBuffer.clear(); + _error = QKeychain::NoError; const QString kck = _account ? AbstractCredentials::keychainKey( _account->url().toString(), @@ -222,24 +226,23 @@ void ReadJob::start() _keychainMigration ? QString() : _account->id() ) : _key; - auto *job = new QKeychain::ReadPasswordJob(_serviceName); + auto *job = new QKeychain::ReadPasswordJob(_serviceName, this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif job->setInsecureFallback(_insecureFallback); job->setKey(kck); connect(job, &QKeychain::Job::finished, this, &KeychainChunk::ReadJob::slotReadJobDone); - _isJobRunning = true; job->start(); } -bool ReadJob::startAwait() +bool ReadJob::exec() { start(); - while (_isJobRunning) { - QApplication::processEvents(QEventLoop::AllEvents, 200); - } + QEventLoop waitLoop; + connect(this, &ReadJob::finished, &waitLoop, &QEventLoop::quit); + waitLoop.exec(); if (error() == NoError) { return true; @@ -273,7 +276,7 @@ void ReadJob::slotReadJobDone(QKeychain::Job *incomingJob) _keychainMigration ? QString() : _account->id() ) : keyWithIndex; - QKeychain::ReadPasswordJob *job = new QKeychain::ReadPasswordJob(_serviceName); + QKeychain::ReadPasswordJob *job = new QKeychain::ReadPasswordJob(_serviceName, this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif @@ -317,8 +320,122 @@ void ReadJob::slotReadJobDone(QKeychain::Job *incomingJob) readJob->deleteLater(); } - _isJobRunning = false; emit finished(this); + + if (_autoDelete) + deleteLater(); +} + +/* +* DeleteJob +*/ +DeleteJob::DeleteJob(Account *account, const QString &key, const bool &keychainMigration, QObject *parent) + : Job(parent) +{ + _account = account; + _key = key; + + _keychainMigration = keychainMigration; +} + +DeleteJob::DeleteJob(const QString &key, QObject *parent) + : DeleteJob(nullptr, key, false, parent) +{ +#ifdef Q_OS_WIN + jobKeyPrependAppName(_key); +#endif +} + +void DeleteJob::start() +{ + _chunkCount = 0; + _error = QKeychain::NoError; + + const QString kck = _account ? AbstractCredentials::keychainKey( + _account->url().toString(), + _key, + _keychainMigration ? QString() : _account->id() + ) : _key; + + auto *job = new QKeychain::DeletePasswordJob(_serviceName, this); +#if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) + addSettingsToJob(_account, job); +#endif + job->setInsecureFallback(_insecureFallback); + job->setKey(kck); + connect(job, &QKeychain::Job::finished, this, &KeychainChunk::DeleteJob::slotDeleteJobDone); + job->start(); +} + +bool DeleteJob::exec() +{ + start(); + + QEventLoop waitLoop; + connect(this, &DeleteJob::finished, &waitLoop, &QEventLoop::quit); + waitLoop.exec(); + + if (error() == NoError) { + return true; + } + + _chunkCount = 0; + if (error() != EntryNotFound) { + qCWarning(lcKeychainChunk) << "DeletePasswordJob failed with" << errorString(); + } + return false; +} + +void DeleteJob::slotDeleteJobDone(QKeychain::Job *incomingJob) +{ + // Errors or next chunk? + auto *deleteJob = static_cast(incomingJob); + + if (deleteJob) { + if (deleteJob->error() == NoError) { + _chunkCount++; + +#if defined(Q_OS_WIN) + // try to delete next chunk + if (_chunkCount < KeychainChunk::MaxChunks) { + const QString keyWithIndex = _key + QString(".") + QString::number(_chunkCount); + const QString kck = _account ? AbstractCredentials::keychainKey( + _account->url().toString(), + keyWithIndex, + _keychainMigration ? QString() : _account->id() + ) : keyWithIndex; + + QKeychain::DeletePasswordJob *job = new QKeychain::DeletePasswordJob(_serviceName, this); +#if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) + addSettingsToJob(_account, job); +#endif + job->setInsecureFallback(_insecureFallback); + job->setKey(kck); + connect(job, &QKeychain::Job::finished, this, &KeychainChunk::DeleteJob::slotDeleteJobDone); + job->start(); + + deleteJob->deleteLater(); + return; + } else { + qCWarning(lcKeychainChunk) << "Maximum chunk count for" << deleteJob->key() << "reached, ignoring after" << KeychainChunk::MaxChunks; + } +#endif + } else { + if (deleteJob->error() != QKeychain::EntryNotFound || + ((deleteJob->error() == QKeychain::EntryNotFound) && _chunkCount == 0)) { + _error = deleteJob->error(); + _errorString = deleteJob->errorString(); + qCWarning(lcKeychainChunk) << "Unable to delete" << deleteJob->key() << "chunk" << QString::number(_chunkCount) << deleteJob->errorString(); + } + } + + deleteJob->deleteLater(); + } + + emit finished(this); + + if (_autoDelete) + deleteLater(); } } // namespace KeychainChunk diff --git a/src/libsync/creds/keychainchunk.h b/src/libsync/creds/keychainchunk.h index 772f69d63..4334dfda5 100644 --- a/src/libsync/creds/keychainchunk.h +++ b/src/libsync/creds/keychainchunk.h @@ -39,7 +39,7 @@ static constexpr int MaxChunks = 10; /* * @brief: Abstract base class for KeychainChunk jobs. */ -class Job : public QObject +class OWNCLOUDSYNC_EXPORT Job : public QObject { Q_OBJECT public: @@ -73,13 +73,29 @@ public: } #endif + /** + * @return Whether this job autodeletes itself once finished() has been emitted. Default is true. + * @see setAutoDelete() + */ + bool autoDelete() const { + return _autoDelete; + } + + /** + * Set whether this job should autodelete itself once finished() has been emitted. + * @see autoDelete() + */ + void setAutoDelete(bool autoDelete) { + _autoDelete = autoDelete; + } + protected: QString _serviceName; Account *_account; QString _key; bool _insecureFallback = false; + bool _autoDelete = true; bool _keychainMigration = false; - bool _isJobRunning = false; QKeychain::Error _error = QKeychain::NoError; QString _errorString; @@ -112,7 +128,7 @@ public: * * @return Returns true on succeess (QKeychain::NoError). */ - bool startAwait(); + bool exec(); signals: void finished(KeychainChunk::WriteJob *incomingJob); @@ -145,7 +161,7 @@ public: * * @return Returns true on succeess (QKeychain::NoError). */ - bool startAwait(); + bool exec(); signals: void finished(KeychainChunk::ReadJob *incomingJob); @@ -159,6 +175,39 @@ private: #endif }; // class ReadJob +/* +* @brief: Simple wrapper class for QKeychain::DeletePasswordJob +*/ +class OWNCLOUDSYNC_EXPORT DeleteJob : public KeychainChunk::Job +{ + Q_OBJECT +public: + DeleteJob(Account *account, const QString &key, const bool &keychainMigration, QObject *parent = nullptr); + DeleteJob(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 exec(); + +signals: + void finished(KeychainChunk::DeleteJob *incomingJob); + +private slots: + void slotDeleteJobDone(QKeychain::Job *incomingJob); +}; // class DeleteJob + } // namespace KeychainChunk } // namespace OCC From 8503226c44f56efb863e34a1dac2dc72a8c8a840 Mon Sep 17 00:00:00 2001 From: Michael Schuster Date: Sat, 27 Jun 2020 04:53:37 +0200 Subject: [PATCH 7/9] Keychain: Use auto deletion in WebFlowCredentials and ConfigFile - Also make use of the new KeychainChunk::DeleteJob Signed-off-by: Michael Schuster --- src/gui/creds/webflowcredentials.cpp | 76 ++++++++-------------------- src/libsync/configfile.cpp | 21 ++++---- 2 files changed, 29 insertions(+), 68 deletions(-) diff --git a/src/gui/creds/webflowcredentials.cpp b/src/gui/creds/webflowcredentials.cpp index 52eef0341..eaaebfc38 100644 --- a/src/gui/creds/webflowcredentials.cpp +++ b/src/gui/creds/webflowcredentials.cpp @@ -240,7 +240,8 @@ void WebFlowCredentials::persist() { if (!_clientSslCertificate.isNull()) { auto *job = new KeychainChunk::WriteJob(_account, _user + clientCertificatePEMC, - _clientSslCertificate.toPem()); + _clientSslCertificate.toPem(), + this); connect(job, &KeychainChunk::WriteJob::finished, this, &WebFlowCredentials::slotWriteClientCertPEMJobDone); job->start(); } else { @@ -251,14 +252,12 @@ void WebFlowCredentials::persist() { void WebFlowCredentials::slotWriteClientCertPEMJobDone(KeychainChunk::WriteJob *writeJob) { - if(writeJob) - writeJob->deleteLater(); - // write ssl key if there is one if (!_clientSslKey.isNull()) { auto *job = new KeychainChunk::WriteJob(_account, _user + clientKeyPEMC, - _clientSslKey.toPem()); + _clientSslKey.toPem(), + this); connect(job, &KeychainChunk::WriteJob::finished, this, &WebFlowCredentials::slotWriteClientKeyPEMJobDone); job->start(); } else { @@ -288,7 +287,8 @@ void WebFlowCredentials::writeSingleClientCaCertPEM() auto *job = new KeychainChunk::WriteJob(_account, _user + clientCaCertificatePEMC + QString::number(index), - cert.toPem()); + cert.toPem(), + this); connect(job, &KeychainChunk::WriteJob::finished, this, &WebFlowCredentials::slotWriteClientCaCertsPEMJobDone); job->start(); } else { @@ -298,9 +298,6 @@ void WebFlowCredentials::writeSingleClientCaCertPEM() void WebFlowCredentials::slotWriteClientKeyPEMJobDone(KeychainChunk::WriteJob *writeJob) { - if(writeJob) - writeJob->deleteLater(); - _clientSslCaCertificatesWriteQueue.clear(); // write ca certs if there are any @@ -323,8 +320,6 @@ void WebFlowCredentials::slotWriteClientCaCertsPEMJobDone(KeychainChunk::WriteJo qCWarning(lcWebFlowCredentials) << "Error while writing client CA cert" << writeJob->errorString(); } - writeJob->deleteLater(); - if (!_clientSslCaCertificatesWriteQueue.isEmpty()) { // next ca cert writeSingleClientCaCertPEM(); @@ -333,7 +328,7 @@ void WebFlowCredentials::slotWriteClientCaCertsPEMJobDone(KeychainChunk::WriteJo } // done storing ca certs, time for the password - auto *job = new WritePasswordJob(Theme::instance()->appName()); + auto *job = new WritePasswordJob(Theme::instance()->appName(), this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif @@ -353,8 +348,6 @@ void WebFlowCredentials::slotWriteJobDone(QKeychain::Job *job) default: qCWarning(lcWebFlowCredentials) << "Error while writing password" << job->errorString(); } - auto *wjob = qobject_cast(job); - wjob->deleteLater(); } void WebFlowCredentials::invalidateToken() { @@ -383,13 +376,9 @@ void WebFlowCredentials::forgetSensitiveData() { return; } - auto *job = new DeletePasswordJob(Theme::instance()->appName()); + auto *job = new DeletePasswordJob(Theme::instance()->appName(), this); job->setInsecureFallback(false); job->setKey(kck); - connect(job, &Job::finished, this, [](QKeychain::Job *job) { - auto *djob = qobject_cast(job); - djob->deleteLater(); - }); job->start(); invalidateToken(); @@ -442,7 +431,8 @@ void WebFlowCredentials::fetchFromKeychainHelper() { // Read client cert from keychain auto *job = new KeychainChunk::ReadJob(_account, _user + clientCertificatePEMC, - _keychainMigration); + _keychainMigration, + this); connect(job, &KeychainChunk::ReadJob::finished, this, &WebFlowCredentials::slotReadClientCertPEMJobDone); job->start(); } @@ -457,12 +447,11 @@ void WebFlowCredentials::slotReadClientCertPEMJobDone(KeychainChunk::ReadJob *re } } - readJob->deleteLater(); - // Load key too auto *job = new KeychainChunk::ReadJob(_account, _user + clientKeyPEMC, - _keychainMigration); + _keychainMigration, + this); connect(job, &KeychainChunk::ReadJob::finished, this, &WebFlowCredentials::slotReadClientKeyPEMJobDone); job->start(); } @@ -489,8 +478,6 @@ void WebFlowCredentials::slotReadClientKeyPEMJobDone(KeychainChunk::ReadJob *rea qCWarning(lcWebFlowCredentials) << "Unable to read client key" << readJob->errorString(); } - readJob->deleteLater(); - // Start fetching client CA certs _clientSslCaCertificates.clear(); @@ -503,7 +490,8 @@ void WebFlowCredentials::readSingleClientCaCertPEM() if (_clientSslCaCertificates.count() < _clientSslCaCertificatesMaxCount) { auto *job = new KeychainChunk::ReadJob(_account, _user + clientCaCertificatePEMC + QString::number(_clientSslCaCertificates.count()), - _keychainMigration); + _keychainMigration, + this); connect(job, &KeychainChunk::ReadJob::finished, this, &WebFlowCredentials::slotReadClientCaCertsPEMJobDone); job->start(); } else { @@ -522,8 +510,6 @@ void WebFlowCredentials::slotReadClientCaCertsPEMJobDone(KeychainChunk::ReadJob _clientSslCaCertificates.append(sslCertificateList.at(0)); } - readJob->deleteLater(); - // try next cert readSingleClientCaCertPEM(); return; @@ -533,8 +519,6 @@ void WebFlowCredentials::slotReadClientCaCertsPEMJobDone(KeychainChunk::ReadJob qCWarning(lcWebFlowCredentials) << "Unable to read client CA cert slot" << QString::number(_clientSslCaCertificates.count()) << readJob->errorString(); } } - - readJob->deleteLater(); } // Now fetch the actual server password @@ -543,7 +527,7 @@ void WebFlowCredentials::slotReadClientCaCertsPEMJobDone(KeychainChunk::ReadJob _user, _keychainMigration ? QString() : _account->id()); - auto *job = new ReadPasswordJob(Theme::instance()->appName()); + auto *job = new ReadPasswordJob(Theme::instance()->appName(), this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif @@ -577,8 +561,6 @@ void WebFlowCredentials::slotReadPasswordJobDone(Job *incomingJob) { } emit fetched(); - job->deleteLater(); - // If keychain data was read from legacy location, wipe these entries and store new ones if (_keychainMigration && _ready) { _keychainMigration = false; @@ -590,15 +572,7 @@ void WebFlowCredentials::slotReadPasswordJobDone(Job *incomingJob) { void WebFlowCredentials::deleteKeychainEntries(bool oldKeychainEntries) { auto startDeleteJob = [this, oldKeychainEntries](QString key) { - auto *job = new DeletePasswordJob(Theme::instance()->appName()); -#if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) - addSettingsToJob(_account, job); -#endif - job->setInsecureFallback(false); - job->setKey(keychainKey(_account->url().toString(), - key, - oldKeychainEntries ? QString() : _account->id())); - job->setAutoDelete(true); + auto *job = new KeychainChunk::DeleteJob(_account, key, oldKeychainEntries, this); job->start(); }; @@ -615,27 +589,17 @@ void WebFlowCredentials::deleteKeychainEntries(bool oldKeychainEntries) { */ if(_account->isRemoteWipeRequested_HACK()) { // <-- FIXME MS@2019-12-07 + + // Also delete key / cert sub-chunks (KeychainChunk takes care of the Windows workaround) + // The first chunk (0) has no suffix, to stay compatible with older versions and non-Windows startDeleteJob(_user + clientKeyPEMC); startDeleteJob(_user + clientCertificatePEMC); + // CA cert slots for (auto i = 0; i < _clientSslCaCertificates.count(); i++) { startDeleteJob(_user + clientCaCertificatePEMC + QString::number(i)); } -#if defined(Q_OS_WIN) - // Also delete key / cert sub-chunks (Windows workaround) - // The first chunk (0) has no suffix, to stay compatible with older versions and non-Windows - for (auto chunk = 1; chunk < KeychainChunk::MaxChunks; chunk++) { - const QString strChunkSuffix = QString(".") + QString::number(chunk); - - startDeleteJob(_user + clientKeyPEMC + strChunkSuffix); - startDeleteJob(_user + clientCertificatePEMC + strChunkSuffix); - - for (auto i = 0; i < _clientSslCaCertificates.count(); i++) { - startDeleteJob(_user + clientCaCertificatePEMC + QString::number(i)); - } - } -#endif // FIXME MS@2019-12-07 --> } // <-- FIXME MS@2019-12-07 diff --git a/src/libsync/configfile.cpp b/src/libsync/configfile.cpp index 299ab09d1..192f7cfbb 100644 --- a/src/libsync/configfile.cpp +++ b/src/libsync/configfile.cpp @@ -654,20 +654,19 @@ void ConfigFile::setProxyType(int proxyType, settings.setValue(QLatin1String(proxyUserC), user); if (pass.isEmpty()) { + // Security: Don't keep password in config file settings.remove(QLatin1String(proxyPassC)); - auto *job = new QKeychain::DeletePasswordJob(Theme::instance()->appName()); - job->setInsecureFallback(false); - job->setKey(keychainProxyPasswordKey()); - job->setAutoDelete(true); - job->start(); + // Delete password from keychain + auto *job = new KeychainChunk::DeleteJob(keychainProxyPasswordKey()); + job->exec(); } else { // Write password to keychain auto *job = new KeychainChunk::WriteJob(keychainProxyPasswordKey(), pass.toUtf8()); - if (job->startAwait()) { + if (job->exec()) { + // Security: Don't keep password in config file settings.remove(QLatin1String(proxyPassC)); } - job->deleteLater(); } } settings.sync(); @@ -752,19 +751,17 @@ QString ConfigFile::proxyPassword() const if (!pass.isEmpty()) { // Security: Migrate password from config file to keychain auto *job = new KeychainChunk::WriteJob(key, pass.toUtf8()); - if (job->startAwait()) { + if (job->exec()) { QSettings settings(configFile(), QSettings::IniFormat); settings.remove(QLatin1String(proxyPassC)); - qCInfo(lcConfigFile()) << "Migrated proxy password to keychain for" << key; + qCInfo(lcConfigFile()) << "Migrated proxy password to keychain"; } - job->deleteLater(); } else { // Read password from keychain auto *job = new KeychainChunk::ReadJob(key); - if (job->startAwait()) { + if (job->exec()) { pass = job->textData(); } - job->deleteLater(); } return pass; From ff631e919f7a70c7b205919dd831a816a14897b9 Mon Sep 17 00:00:00 2001 From: Michael Schuster Date: Tue, 30 Jun 2020 06:02:58 +0200 Subject: [PATCH 8/9] ProxyAuthHandler: Add template member function execAwait to avoid code duplication Signed-off-by: Michael Schuster --- src/gui/proxyauthhandler.cpp | 50 +++++++++++++++++++++--------------- src/gui/proxyauthhandler.h | 6 +++++ 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/src/gui/proxyauthhandler.cpp b/src/gui/proxyauthhandler.cpp index 010093b54..0506572fb 100644 --- a/src/gui/proxyauthhandler.cpp +++ b/src/gui/proxyauthhandler.cpp @@ -24,6 +24,7 @@ #include using namespace OCC; +using namespace QKeychain; Q_LOGGING_CATEGORY(lcProxy, "nextcloud.gui.credentials.proxy", QtInfoMsg) @@ -139,24 +140,22 @@ void ProxyAuthHandler::slotSenderDestroyed(QObject *obj) bool ProxyAuthHandler::getCredsFromDialog() { - QEventLoop waitLoop; - // Open the credentials dialog if (!_waitingForDialog) { _dialog->reset(); _dialog->setProxyAddress(_proxy); - connect(_dialog, &QDialog::finished, &waitLoop, &QEventLoop::quit); _dialog->open(); } // This function can be reentered while the dialog is open. // If that's the case, continue processing the dialog until // it's done. - ++_waitingForDialog; - if (_dialog) { - waitLoop.exec(QEventLoop::ExcludeSocketNotifiers); + if(_dialog) { + execAwait(_dialog.data(), + &QDialog::finished, + _waitingForDialog, + QEventLoop::ExcludeSocketNotifiers); } - --_waitingForDialog; if (_dialog && _dialog->result() == QDialog::Accepted) { qCInfo(lcProxy) << "got creds for" << _proxy << "from dialog"; @@ -167,11 +166,25 @@ bool ProxyAuthHandler::getCredsFromDialog() return false; } +template +void ProxyAuthHandler::execAwait(const T *sender, + PointerToMemberFunction signal, + int &counter, + const QEventLoop::ProcessEventsFlags flags) +{ + if(!sender) + return; + + QEventLoop waitLoop; + connect(sender, signal, &waitLoop, &QEventLoop::quit); + + ++counter; + waitLoop.exec(flags); + --counter; +} + bool ProxyAuthHandler::getCredsFromKeychain() { - using namespace QKeychain; - QEventLoop waitLoop; - if (_waitingForDialog) { return false; } @@ -189,7 +202,6 @@ bool ProxyAuthHandler::getCredsFromKeychain() _readPasswordJob->setInsecureFallback(false); _readPasswordJob->setKey(keychainPasswordKey()); _readPasswordJob->setAutoDelete(false); - connect(_readPasswordJob.data(), &QKeychain::Job::finished, &waitLoop, &QEventLoop::quit); _readPasswordJob->start(); } @@ -197,9 +209,9 @@ bool ProxyAuthHandler::getCredsFromKeychain() // This really needs the counter and the flag here, because otherwise we get // bad behavior when we reenter this code after the flag has been switched // but before the while loop has finished. - ++_waitingForKeychain; - waitLoop.exec(); - --_waitingForKeychain; + execAwait(_readPasswordJob.data(), + &QKeychain::Job::finished, + _waitingForKeychain); if (_readPasswordJob->error() == NoError) { qCInfo(lcProxy) << "got creds for" << _proxy << "from keychain"; @@ -216,9 +228,6 @@ bool ProxyAuthHandler::getCredsFromKeychain() void ProxyAuthHandler::storeCredsInKeychain() { - using namespace QKeychain; - QEventLoop waitLoop; - if (_waitingForKeychain) { return; } @@ -233,12 +242,11 @@ void ProxyAuthHandler::storeCredsInKeychain() job->setKey(keychainPasswordKey()); job->setTextData(_password); job->setAutoDelete(false); - connect(job, &QKeychain::Job::finished, &waitLoop, &QEventLoop::quit); job->start(); - ++_waitingForKeychain; - waitLoop.exec(); - --_waitingForKeychain; + execAwait(job, + &QKeychain::Job::finished, + _waitingForKeychain); job->deleteLater(); if (job->error() != NoError) { diff --git a/src/gui/proxyauthhandler.h b/src/gui/proxyauthhandler.h index ccc6d824b..e1a291411 100644 --- a/src/gui/proxyauthhandler.h +++ b/src/gui/proxyauthhandler.h @@ -71,6 +71,12 @@ private: /// Stores the current credentials in the keychain. void storeCredsInKeychain(); + template + void execAwait(const T *sender, + PointerToMemberFunction signal, + int &counter, + const QEventLoop::ProcessEventsFlags flags = QEventLoop::AllEvents); + QString keychainUsernameKey() const; QString keychainPasswordKey() const; From f4d83d02f6ed62947e00879a0100ce778030815c Mon Sep 17 00:00:00 2001 From: Michael Schuster Date: Tue, 30 Jun 2020 16:00:29 +0200 Subject: [PATCH 9/9] Cleanup auto pointers and qobject casts, refactor KeychainChunk Signed-off-by: Michael Schuster --- src/gui/creds/webflowcredentials.cpp | 60 +++---- src/gui/proxyauthhandler.cpp | 7 +- src/libsync/configfile.cpp | 8 +- src/libsync/creds/keychainchunk.cpp | 224 ++++++++++++++++----------- src/libsync/creds/keychainchunk.h | 37 ++--- 5 files changed, 182 insertions(+), 154 deletions(-) diff --git a/src/gui/creds/webflowcredentials.cpp b/src/gui/creds/webflowcredentials.cpp index eaaebfc38..58beb358c 100644 --- a/src/gui/creds/webflowcredentials.cpp +++ b/src/gui/creds/webflowcredentials.cpp @@ -145,7 +145,7 @@ void WebFlowCredentials::fetchFromKeychain() { void WebFlowCredentials::askFromUser() { // Determine if the old flow has to be used (GS for now) // Do a DetermineAuthTypeJob to make sure that the server is still using Flow2 - auto *job = new DetermineAuthTypeJob(_account->sharedFromThis(), this); + auto job = new DetermineAuthTypeJob(_account->sharedFromThis(), this); connect(job, &DetermineAuthTypeJob::authType, [this](DetermineAuthTypeJob::AuthType type) { // LoginFlowV2 > WebViewFlow > OAuth > Shib > Basic bool useFlow2 = (type != DetermineAuthTypeJob::WebViewFlow); @@ -238,10 +238,10 @@ void WebFlowCredentials::persist() { // write cert if there is one if (!_clientSslCertificate.isNull()) { - auto *job = new KeychainChunk::WriteJob(_account, - _user + clientCertificatePEMC, - _clientSslCertificate.toPem(), - this); + auto job = new KeychainChunk::WriteJob(_account, + _user + clientCertificatePEMC, + _clientSslCertificate.toPem(), + this); connect(job, &KeychainChunk::WriteJob::finished, this, &WebFlowCredentials::slotWriteClientCertPEMJobDone); job->start(); } else { @@ -254,10 +254,10 @@ void WebFlowCredentials::slotWriteClientCertPEMJobDone(KeychainChunk::WriteJob * { // write ssl key if there is one if (!_clientSslKey.isNull()) { - auto *job = new KeychainChunk::WriteJob(_account, - _user + clientKeyPEMC, - _clientSslKey.toPem(), - this); + auto job = new KeychainChunk::WriteJob(_account, + _user + clientKeyPEMC, + _clientSslKey.toPem(), + this); connect(job, &KeychainChunk::WriteJob::finished, this, &WebFlowCredentials::slotWriteClientKeyPEMJobDone); job->start(); } else { @@ -285,10 +285,10 @@ void WebFlowCredentials::writeSingleClientCaCertPEM() return; } - auto *job = new KeychainChunk::WriteJob(_account, - _user + clientCaCertificatePEMC + QString::number(index), - cert.toPem(), - this); + auto job = new KeychainChunk::WriteJob(_account, + _user + clientCaCertificatePEMC + QString::number(index), + cert.toPem(), + this); connect(job, &KeychainChunk::WriteJob::finished, this, &WebFlowCredentials::slotWriteClientCaCertsPEMJobDone); job->start(); } else { @@ -328,7 +328,7 @@ void WebFlowCredentials::slotWriteClientCaCertsPEMJobDone(KeychainChunk::WriteJo } // done storing ca certs, time for the password - auto *job = new WritePasswordJob(Theme::instance()->appName(), this); + auto job = new WritePasswordJob(Theme::instance()->appName(), this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif @@ -376,7 +376,7 @@ void WebFlowCredentials::forgetSensitiveData() { return; } - auto *job = new DeletePasswordJob(Theme::instance()->appName(), this); + auto job = new DeletePasswordJob(Theme::instance()->appName(), this); job->setInsecureFallback(false); job->setKey(kck); job->start(); @@ -429,10 +429,10 @@ void WebFlowCredentials::slotFinished(QNetworkReply *reply) { void WebFlowCredentials::fetchFromKeychainHelper() { // Read client cert from keychain - auto *job = new KeychainChunk::ReadJob(_account, - _user + clientCertificatePEMC, - _keychainMigration, - this); + auto job = new KeychainChunk::ReadJob(_account, + _user + clientCertificatePEMC, + _keychainMigration, + this); connect(job, &KeychainChunk::ReadJob::finished, this, &WebFlowCredentials::slotReadClientCertPEMJobDone); job->start(); } @@ -448,10 +448,10 @@ void WebFlowCredentials::slotReadClientCertPEMJobDone(KeychainChunk::ReadJob *re } // Load key too - auto *job = new KeychainChunk::ReadJob(_account, - _user + clientKeyPEMC, - _keychainMigration, - this); + auto job = new KeychainChunk::ReadJob(_account, + _user + clientKeyPEMC, + _keychainMigration, + this); connect(job, &KeychainChunk::ReadJob::finished, this, &WebFlowCredentials::slotReadClientKeyPEMJobDone); job->start(); } @@ -488,10 +488,10 @@ void WebFlowCredentials::readSingleClientCaCertPEM() { // try to fetch a client ca cert if (_clientSslCaCertificates.count() < _clientSslCaCertificatesMaxCount) { - auto *job = new KeychainChunk::ReadJob(_account, - _user + clientCaCertificatePEMC + QString::number(_clientSslCaCertificates.count()), - _keychainMigration, - this); + auto job = new KeychainChunk::ReadJob(_account, + _user + clientCaCertificatePEMC + QString::number(_clientSslCaCertificates.count()), + _keychainMigration, + this); connect(job, &KeychainChunk::ReadJob::finished, this, &WebFlowCredentials::slotReadClientCaCertsPEMJobDone); job->start(); } else { @@ -527,7 +527,7 @@ void WebFlowCredentials::slotReadClientCaCertsPEMJobDone(KeychainChunk::ReadJob _user, _keychainMigration ? QString() : _account->id()); - auto *job = new ReadPasswordJob(Theme::instance()->appName(), this); + auto job = new ReadPasswordJob(Theme::instance()->appName(), this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif @@ -538,7 +538,7 @@ void WebFlowCredentials::slotReadClientCaCertsPEMJobDone(KeychainChunk::ReadJob } void WebFlowCredentials::slotReadPasswordJobDone(Job *incomingJob) { - auto *job = qobject_cast(incomingJob); + auto job = qobject_cast(incomingJob); QKeychain::Error error = job->error(); // If we could not find the entry try the old entries @@ -572,7 +572,7 @@ void WebFlowCredentials::slotReadPasswordJobDone(Job *incomingJob) { void WebFlowCredentials::deleteKeychainEntries(bool oldKeychainEntries) { auto startDeleteJob = [this, oldKeychainEntries](QString key) { - auto *job = new KeychainChunk::DeleteJob(_account, key, oldKeychainEntries, this); + auto job = new KeychainChunk::DeleteJob(_account, key, oldKeychainEntries, this); job->start(); }; diff --git a/src/gui/proxyauthhandler.cpp b/src/gui/proxyauthhandler.cpp index 0506572fb..9c2703bda 100644 --- a/src/gui/proxyauthhandler.cpp +++ b/src/gui/proxyauthhandler.cpp @@ -82,7 +82,7 @@ void ProxyAuthHandler::handleProxyAuthenticationRequired( // Find the responsible QNAM if possible. QNetworkAccessManager *sending_qnam = nullptr; QWeakPointer qnam_alive; - if (auto *account = qobject_cast(sender())) { + if (auto account = qobject_cast(sender())) { // Since we go into an event loop, it's possible for the account's qnam // to be destroyed before we get back. We can use this to check for its // liveness. @@ -172,8 +172,9 @@ void ProxyAuthHandler::execAwait(const T *sender, int &counter, const QEventLoop::ProcessEventsFlags flags) { - if(!sender) + if (!sender) { return; + } QEventLoop waitLoop; connect(sender, signal, &waitLoop, &QEventLoop::quit); @@ -236,7 +237,7 @@ void ProxyAuthHandler::storeCredsInKeychain() _settings->setValue(keychainUsernameKey(), _username); - auto *job = new WritePasswordJob(Theme::instance()->appName(), this); + auto job = new WritePasswordJob(Theme::instance()->appName(), this); job->setSettings(_settings.data()); job->setInsecureFallback(false); job->setKey(keychainPasswordKey()); diff --git a/src/libsync/configfile.cpp b/src/libsync/configfile.cpp index 192f7cfbb..c4e81110c 100644 --- a/src/libsync/configfile.cpp +++ b/src/libsync/configfile.cpp @@ -658,11 +658,11 @@ void ConfigFile::setProxyType(int proxyType, settings.remove(QLatin1String(proxyPassC)); // Delete password from keychain - auto *job = new KeychainChunk::DeleteJob(keychainProxyPasswordKey()); + auto job = new KeychainChunk::DeleteJob(keychainProxyPasswordKey()); job->exec(); } else { // Write password to keychain - auto *job = new KeychainChunk::WriteJob(keychainProxyPasswordKey(), pass.toUtf8()); + auto job = new KeychainChunk::WriteJob(keychainProxyPasswordKey(), pass.toUtf8()); if (job->exec()) { // Security: Don't keep password in config file settings.remove(QLatin1String(proxyPassC)); @@ -750,7 +750,7 @@ QString ConfigFile::proxyPassword() const if (!pass.isEmpty()) { // Security: Migrate password from config file to keychain - auto *job = new KeychainChunk::WriteJob(key, pass.toUtf8()); + auto job = new KeychainChunk::WriteJob(key, pass.toUtf8()); if (job->exec()) { QSettings settings(configFile(), QSettings::IniFormat); settings.remove(QLatin1String(proxyPassC)); @@ -758,7 +758,7 @@ QString ConfigFile::proxyPassword() const } } else { // Read password from keychain - auto *job = new KeychainChunk::ReadJob(key); + auto job = new KeychainChunk::ReadJob(key); if (job->exec()) { pass = job->textData(); } diff --git a/src/libsync/creds/keychainchunk.cpp b/src/libsync/creds/keychainchunk.cpp index 5dc0ff2a2..cfa80e7e1 100644 --- a/src/libsync/creds/keychainchunk.cpp +++ b/src/libsync/creds/keychainchunk.cpp @@ -70,6 +70,48 @@ Job::~Job() _chunkBuffer.clear(); } +QKeychain::Error Job::error() const +{ + return _error; +} + +QString Job::errorString() const +{ + return _errorString; +} + +QByteArray Job::binaryData() const +{ + return _chunkBuffer; +} + +QString Job::textData() const +{ + return _chunkBuffer; +} + +bool Job::insecureFallback() const +{ + return _insecureFallback; +} + +#if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) +void Job::setInsecureFallback(bool insecureFallback) +{ + _insecureFallback = insecureFallback; +} +#endif + +bool Job::autoDelete() const +{ + return _autoDelete; +} + +void Job::setAutoDelete(bool autoDelete) +{ + _autoDelete = autoDelete; +} + /* * WriteJob */ @@ -118,9 +160,9 @@ bool WriteJob::exec() void WriteJob::slotWriteJobDone(QKeychain::Job *incomingJob) { - auto *writeJob = static_cast(incomingJob); + auto writeJob = qobject_cast(incomingJob); - // errors? + // Errors? (writeJob can be nullptr here, see: WriteJob::start) if (writeJob) { _error = writeJob->error(); _errorString = writeJob->errorString(); @@ -157,8 +199,9 @@ void WriteJob::slotWriteJobDone(QKeychain::Job *incomingJob) emit finished(this); - if (_autoDelete) + if (_autoDelete) { deleteLater(); + } return; } @@ -169,7 +212,7 @@ void WriteJob::slotWriteJobDone(QKeychain::Job *incomingJob) _account->id() ) : keyWithIndex; - auto *job = new QKeychain::WritePasswordJob(_serviceName, this); + auto job = new QKeychain::WritePasswordJob(_serviceName, this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif @@ -184,8 +227,9 @@ void WriteJob::slotWriteJobDone(QKeychain::Job *incomingJob) } else { emit finished(this); - if (_autoDelete) + if (_autoDelete) { deleteLater(); + } } writeJob->deleteLater(); @@ -194,7 +238,7 @@ void WriteJob::slotWriteJobDone(QKeychain::Job *incomingJob) /* * ReadJob */ -ReadJob::ReadJob(Account *account, const QString &key, const bool &keychainMigration, QObject *parent) +ReadJob::ReadJob(Account *account, const QString &key, bool keychainMigration, QObject *parent) : Job(parent) { _account = account; @@ -226,7 +270,7 @@ void ReadJob::start() _keychainMigration ? QString() : _account->id() ) : _key; - auto *job = new QKeychain::ReadPasswordJob(_serviceName, this); + auto job = new QKeychain::ReadPasswordJob(_serviceName, this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif @@ -259,77 +303,77 @@ bool ReadJob::exec() void ReadJob::slotReadJobDone(QKeychain::Job *incomingJob) { // Errors or next chunk? - auto *readJob = static_cast(incomingJob); + auto readJob = qobject_cast(incomingJob); + Q_ASSERT(readJob); - if (readJob) { - if (readJob->error() == NoError && readJob->binaryData().length() > 0) { - _chunkBuffer.append(readJob->binaryData()); - _chunkCount++; + if (readJob->error() == NoError && !readJob->binaryData().isEmpty()) { + _chunkBuffer.append(readJob->binaryData()); + _chunkCount++; #if defined(Q_OS_WIN) - // try to fetch next chunk - if (_chunkCount < KeychainChunk::MaxChunks) { - const QString keyWithIndex = _key + QString(".") + QString::number(_chunkCount); - const QString kck = _account ? AbstractCredentials::keychainKey( - _account->url().toString(), - keyWithIndex, - _keychainMigration ? QString() : _account->id() - ) : keyWithIndex; + // try to fetch next chunk + if (_chunkCount < KeychainChunk::MaxChunks) { + 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, this); + auto job = new QKeychain::ReadPasswordJob(_serviceName, this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) - addSettingsToJob(_account, job); + addSettingsToJob(_account, job); #endif - job->setInsecureFallback(_insecureFallback); - job->setKey(kck); - connect(job, &QKeychain::Job::finished, this, &KeychainChunk::ReadJob::slotReadJobDone); - job->start(); + job->setInsecureFallback(_insecureFallback); + job->setKey(kck); + connect(job, &QKeychain::Job::finished, this, &KeychainChunk::ReadJob::slotReadJobDone); + job->start(); + readJob->deleteLater(); + return; + } else { + qCWarning(lcKeychainChunk) << "Maximum chunk count for" << readJob->key() << "reached, ignoring after" << KeychainChunk::MaxChunks; + } +#endif + } else { +#if defined(Q_OS_UNIX) && !defined(Q_OS_MAC) + if (!readJob->insecureFallback()) { // If insecureFallback is set, the next test would be pointless + if (_retryOnKeyChainError && (readJob->error() == QKeychain::NoBackendAvailable + || readJob->error() == QKeychain::OtherError)) { + // Could be that the backend was not yet available. Wait some extra seconds. + // (Issues #4274 and #6522) + // (For kwallet, the error is OtherError instead of NoBackendAvailable, maybe a bug in QtKeychain) + qCInfo(lcKeychainChunk) << "Backend unavailable (yet?) Retrying in a few seconds." << readJob->errorString(); + QTimer::singleShot(10000, this, &ReadJob::start); + _retryOnKeyChainError = false; readJob->deleteLater(); return; - } else { - qCWarning(lcKeychainChunk) << "Maximum chunk count for" << readJob->key() << "reached, ignoring after" << KeychainChunk::MaxChunks; - } -#endif - } else { -#if defined(Q_OS_UNIX) && !defined(Q_OS_MAC) - if (!readJob->insecureFallback()) { // If insecureFallback is set, the next test would be pointless - if (_retryOnKeyChainError && (readJob->error() == QKeychain::NoBackendAvailable - || readJob->error() == QKeychain::OtherError)) { - // Could be that the backend was not yet available. Wait some extra seconds. - // (Issues #4274 and #6522) - // (For kwallet, the error is OtherError instead of NoBackendAvailable, maybe a bug in QtKeychain) - qCInfo(lcKeychainChunk) << "Backend unavailable (yet?) Retrying in a few seconds." << readJob->errorString(); - QTimer::singleShot(10000, this, &ReadJob::start); - _retryOnKeyChainError = false; - readJob->deleteLater(); - return; - } - _retryOnKeyChainError = false; - } -#endif - - if (readJob->error() != QKeychain::EntryNotFound || - ((readJob->error() == QKeychain::EntryNotFound) && _chunkCount == 0)) { - _error = readJob->error(); - _errorString = readJob->errorString(); - qCWarning(lcKeychainChunk) << "Unable to read" << readJob->key() << "chunk" << QString::number(_chunkCount) << readJob->errorString(); } + _retryOnKeyChainError = false; } +#endif - readJob->deleteLater(); + if (readJob->error() != QKeychain::EntryNotFound || + ((readJob->error() == QKeychain::EntryNotFound) && _chunkCount == 0)) { + _error = readJob->error(); + _errorString = readJob->errorString(); + qCWarning(lcKeychainChunk) << "Unable to read" << readJob->key() << "chunk" << QString::number(_chunkCount) << readJob->errorString(); + } } + readJob->deleteLater(); + emit finished(this); - if (_autoDelete) + if (_autoDelete) { deleteLater(); + } } /* * DeleteJob */ -DeleteJob::DeleteJob(Account *account, const QString &key, const bool &keychainMigration, QObject *parent) +DeleteJob::DeleteJob(Account *account, const QString &key, bool keychainMigration, QObject *parent) : Job(parent) { _account = account; @@ -357,7 +401,7 @@ void DeleteJob::start() _keychainMigration ? QString() : _account->id() ) : _key; - auto *job = new QKeychain::DeletePasswordJob(_serviceName, this); + auto job = new QKeychain::DeletePasswordJob(_serviceName, this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif @@ -389,53 +433,53 @@ bool DeleteJob::exec() void DeleteJob::slotDeleteJobDone(QKeychain::Job *incomingJob) { // Errors or next chunk? - auto *deleteJob = static_cast(incomingJob); + auto deleteJob = qobject_cast(incomingJob); + Q_ASSERT(deleteJob); - if (deleteJob) { - if (deleteJob->error() == NoError) { - _chunkCount++; + if (deleteJob->error() == NoError) { + _chunkCount++; #if defined(Q_OS_WIN) - // try to delete next chunk - if (_chunkCount < KeychainChunk::MaxChunks) { - const QString keyWithIndex = _key + QString(".") + QString::number(_chunkCount); - const QString kck = _account ? AbstractCredentials::keychainKey( - _account->url().toString(), - keyWithIndex, - _keychainMigration ? QString() : _account->id() - ) : keyWithIndex; + // try to delete next chunk + if (_chunkCount < KeychainChunk::MaxChunks) { + const QString keyWithIndex = _key + QString(".") + QString::number(_chunkCount); + const QString kck = _account ? AbstractCredentials::keychainKey( + _account->url().toString(), + keyWithIndex, + _keychainMigration ? QString() : _account->id() + ) : keyWithIndex; - QKeychain::DeletePasswordJob *job = new QKeychain::DeletePasswordJob(_serviceName, this); + auto job = new QKeychain::DeletePasswordJob(_serviceName, this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) - addSettingsToJob(_account, job); + addSettingsToJob(_account, job); #endif - job->setInsecureFallback(_insecureFallback); - job->setKey(kck); - connect(job, &QKeychain::Job::finished, this, &KeychainChunk::DeleteJob::slotDeleteJobDone); - job->start(); + job->setInsecureFallback(_insecureFallback); + job->setKey(kck); + connect(job, &QKeychain::Job::finished, this, &KeychainChunk::DeleteJob::slotDeleteJobDone); + job->start(); - deleteJob->deleteLater(); - return; - } else { - qCWarning(lcKeychainChunk) << "Maximum chunk count for" << deleteJob->key() << "reached, ignoring after" << KeychainChunk::MaxChunks; - } -#endif + deleteJob->deleteLater(); + return; } else { - if (deleteJob->error() != QKeychain::EntryNotFound || - ((deleteJob->error() == QKeychain::EntryNotFound) && _chunkCount == 0)) { - _error = deleteJob->error(); - _errorString = deleteJob->errorString(); - qCWarning(lcKeychainChunk) << "Unable to delete" << deleteJob->key() << "chunk" << QString::number(_chunkCount) << deleteJob->errorString(); - } + qCWarning(lcKeychainChunk) << "Maximum chunk count for" << deleteJob->key() << "reached, ignoring after" << KeychainChunk::MaxChunks; + } +#endif + } else { + if (deleteJob->error() != QKeychain::EntryNotFound || + ((deleteJob->error() == QKeychain::EntryNotFound) && _chunkCount == 0)) { + _error = deleteJob->error(); + _errorString = deleteJob->errorString(); + qCWarning(lcKeychainChunk) << "Unable to delete" << deleteJob->key() << "chunk" << QString::number(_chunkCount) << deleteJob->errorString(); } - - deleteJob->deleteLater(); } + deleteJob->deleteLater(); + emit finished(this); - if (_autoDelete) + if (_autoDelete) { deleteLater(); + } } } // namespace KeychainChunk diff --git a/src/libsync/creds/keychainchunk.h b/src/libsync/creds/keychainchunk.h index 4334dfda5..1df27e75a 100644 --- a/src/libsync/creds/keychainchunk.h +++ b/src/libsync/creds/keychainchunk.h @@ -47,47 +47,30 @@ public: virtual ~Job(); - const QKeychain::Error error() const { - return _error; - } - const QString errorString() const { - return _errorString; - } + QKeychain::Error error() const; + QString errorString() const; - QByteArray binaryData() const { - return _chunkBuffer; - } - QString textData() const { - return _chunkBuffer; - } + QByteArray binaryData() const; + QString textData() const; - const bool insecureFallback() const { - return _insecureFallback; - } + bool insecureFallback() const; // If we use it but don't support insecure fallback, give us nice compilation errors ;p #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) - void setInsecureFallback(const bool &insecureFallback) - { - _insecureFallback = insecureFallback; - } + void setInsecureFallback(bool insecureFallback); #endif /** * @return Whether this job autodeletes itself once finished() has been emitted. Default is true. * @see setAutoDelete() */ - bool autoDelete() const { - return _autoDelete; - } + bool autoDelete() const; /** * Set whether this job should autodelete itself once finished() has been emitted. * @see autoDelete() */ - void setAutoDelete(bool autoDelete) { - _autoDelete = autoDelete; - } + void setAutoDelete(bool autoDelete); protected: QString _serviceName; @@ -144,7 +127,7 @@ class OWNCLOUDSYNC_EXPORT ReadJob : public KeychainChunk::Job { Q_OBJECT public: - ReadJob(Account *account, const QString &key, const bool &keychainMigration, QObject *parent = nullptr); + ReadJob(Account *account, const QString &key, bool keychainMigration, QObject *parent = nullptr); ReadJob(const QString &key, QObject *parent = nullptr); /** @@ -182,7 +165,7 @@ class OWNCLOUDSYNC_EXPORT DeleteJob : public KeychainChunk::Job { Q_OBJECT public: - DeleteJob(Account *account, const QString &key, const bool &keychainMigration, QObject *parent = nullptr); + DeleteJob(Account *account, const QString &key, bool keychainMigration, QObject *parent = nullptr); DeleteJob(const QString &key, QObject *parent = nullptr); /**