From 00a5096d108d431f3e0ec71e8c4ea260e151df15 Mon Sep 17 00:00:00 2001 From: Tobias Fella Date: Wed, 17 Jul 2024 23:01:37 +0200 Subject: [PATCH 1/8] Get rid of nested QEventLoop when loading pickling key --- Quotient/connection.cpp | 15 +- Quotient/connectionencryptiondata_p.cpp | 175 ++++++++++++------------ Quotient/connectionencryptiondata_p.h | 3 +- 3 files changed, 100 insertions(+), 93 deletions(-) diff --git a/Quotient/connection.cpp b/Quotient/connection.cpp index 77b112c80..8a5aac23a 100644 --- a/Quotient/connection.cpp +++ b/Quotient/connection.cpp @@ -320,13 +320,14 @@ void Connection::Private::completeSetup(const QString& mxId, bool mock) } if (useEncryption) { - if (auto&& maybeEncryptionData = - _impl::ConnectionEncryptionData::setup(q, mock)) { - encryptionData = std::move(*maybeEncryptionData); - } else { - useEncryption = false; - emit q->encryptionChanged(false); - } + _impl::ConnectionEncryptionData::setup(q, [this](auto&& maybeEncryptionData){ + if (maybeEncryptionData) { + encryptionData = std::move(*maybeEncryptionData); + } else { + useEncryption = false; + emit q->encryptionChanged(false); + } + }, mock); } else qCInfo(E2EE) << "End-to-end encryption (E2EE) support is off for" << q->objectName(); diff --git a/Quotient/connectionencryptiondata_p.cpp b/Quotient/connectionencryptiondata_p.cpp index fcff09692..4aa5b4b6d 100644 --- a/Quotient/connectionencryptiondata_p.cpp +++ b/Quotient/connectionencryptiondata_p.cpp @@ -18,102 +18,109 @@ using namespace Quotient; using namespace Quotient::_impl; -Expected setupPicklingKey(const QString& id, - bool mock) +void setupPicklingKey(const QString& id, bool mock, std::function)> then) { if (mock) { qInfo(E2EE) << "Using a mock pickling key"; - return PicklingKey::generate(); + then(PicklingKey::generate()); + return; } // TODO: Rewrite the whole thing in an async way to get rid of nested event // loops using namespace QKeychain; const auto keychainId = id + "-Pickle"_ls; - ReadPasswordJob readJob(qAppName()); - readJob.setAutoDelete(false); - readJob.setKey(keychainId); - QEventLoop readLoop; - QObject::connect(&readJob, &Job::finished, &readLoop, &QEventLoop::quit); - readJob.start(); - readLoop.exec(); - - if (readJob.error() == Error::NoError) { - auto&& data = readJob.binaryData(); - if (data.size() == PicklingKey::extent) { - qDebug(E2EE) << "Successfully loaded pickling key from keychain"; - return PicklingKey::fromByteArray(std::move(data)); + auto readJob = new ReadPasswordJob(qAppName()); + readJob->setAutoDelete(true); + qWarning() << "app" << qAppName() << "id" << keychainId; + readJob->setKey(keychainId); + QObject::connect(readJob, &Job::finished, readJob, [readJob, then, id, keychainId](){ + if (readJob->error() == Error::NoError) { + auto&& data = readJob->binaryData(); + if (data.size() == PicklingKey::extent) { + qDebug(E2EE) << "Successfully loaded pickling key from keychain"; + then(PicklingKey::fromByteArray(std::move(data))); + return; + } + qCritical(E2EE) << "The loaded pickling key for" << id + << "has length" << data.size() + << "but the library expected" << PicklingKey::extent; + then(Error::OtherError); + return; } - qCritical(E2EE) << "The loaded pickling key for" << id - << "has length" << data.size() - << "but the library expected" << PicklingKey::extent; - return Error::OtherError; - } - if (readJob.error() == Error::EntryNotFound) { - auto&& picklingKey = PicklingKey::generate(); - WritePasswordJob writeJob(qAppName()); - writeJob.setAutoDelete(false); - writeJob.setKey(keychainId); - writeJob.setBinaryData(picklingKey.viewAsByteArray()); - QEventLoop writeLoop; - QObject::connect(&writeJob, &Job::finished, &writeLoop, - &QEventLoop::quit); - writeJob.start(); - writeLoop.exec(); - - if (writeJob.error() == Error::NoError) - return std::move(picklingKey); - - qCritical(E2EE) << "Could not save pickling key to keychain: " - << writeJob.errorString(); - return writeJob.error(); - } - qWarning(E2EE) << "Error loading pickling key - please fix your keychain:" - << readJob.errorString(); - return readJob.error(); -} - -std::optional> -ConnectionEncryptionData::setup(Connection* connection, bool mock) -{ - if (auto&& maybePicklingKey = setupPicklingKey(connection->userId(), mock)) { - auto&& encryptionData = std::make_unique( - connection, std::move(*maybePicklingKey)); - if (mock) { - encryptionData->database.clear(); - encryptionData->olmAccount.setupNewAccount(); - return std::move(encryptionData); + if (readJob->error() == Error::EntryNotFound) { + auto&& picklingKey = PicklingKey::generate(); + auto writeJob = new WritePasswordJob(qAppName()); + writeJob->setAutoDelete(true); + writeJob->setKey(keychainId); + writeJob->setBinaryData(picklingKey.viewAsByteArray()); + QObject::connect(writeJob, &Job::finished, writeJob, [writeJob, then, &picklingKey](){ + writeJob->start(); + + if (writeJob->error() == Error::NoError) { + then(std::move(picklingKey)); + return; + } + + qCritical(E2EE) << "Could not save pickling key to keychain: " + << writeJob->errorString(); + then(writeJob->error()); + }); } - if (const auto outcome = encryptionData->database.setupOlmAccount( - encryptionData->olmAccount)) { - // account already existing or there's an error unpickling it - if (outcome == OLM_SUCCESS) - return std::move(encryptionData); - - qCritical(E2EE) << "Could not unpickle Olm account for" - << connection->objectName(); - return {}; + qWarning(E2EE) << "Error loading pickling key - please fix your keychain:" + << readJob->errorString(); + then(readJob->error()); + return; + }); + readJob->start(); +} + +void ConnectionEncryptionData::setup(Connection* connection, std::function>)> then, bool mock) +{ + setupPicklingKey(connection->userId(), mock, [connection, then, mock](Expected maybePicklingKey){ + if(maybePicklingKey) { + auto&& encryptionData = std::make_unique( + connection, std::move(*maybePicklingKey)); + if (mock) { + encryptionData->database.clear(); + encryptionData->olmAccount.setupNewAccount(); + then(std::move(encryptionData)); + return; + } + if (const auto outcome = encryptionData->database.setupOlmAccount( + encryptionData->olmAccount)) { + // account already existing or there's an error unpickling it + if (outcome == OLM_SUCCESS) { + then(std::move(encryptionData)); + return; + } + + qCritical(E2EE) << "Could not unpickle Olm account for" + << connection->objectName(); + then(std::nullopt); + return; + } + // A new account has been created + auto job = connection->callApi( + encryptionData->olmAccount.deviceKeys()); + // eData is meant to have the same scope as connection so it's safe + // to pass an unguarded pointer to encryption data here + QObject::connect(job, &BaseJob::success, connection, + [connection, eData = encryptionData.get()] { + eData->trackedUsers += connection->userId(); + eData->outdatedUsers += connection->userId(); + eData->encryptionUpdateRequired = true; + }); + QObject::connect(job, &BaseJob::failure, connection, [job] { + qCWarning(E2EE) + << "Failed to upload device keys:" << job->errorString(); + }); + then(std::move(encryptionData)); + return; } - // A new account has been created - auto job = connection->callApi( - encryptionData->olmAccount.deviceKeys()); - // eData is meant to have the same scope as connection so it's safe - // to pass an unguarded pointer to encryption data here - QObject::connect(job, &BaseJob::success, connection, - [connection, eData = encryptionData.get()] { - eData->trackedUsers += connection->userId(); - eData->outdatedUsers += connection->userId(); - eData->encryptionUpdateRequired = true; - }); - QObject::connect(job, &BaseJob::failure, connection, [job] { - qCWarning(E2EE) - << "Failed to upload device keys:" << job->errorString(); - }); - return std::move(encryptionData); - } - qCritical(E2EE) << "Could not load or initialise a pickling key for" - << connection->objectName(); - return {}; + qCritical(E2EE) << "Could not load or initialise a pickling key for" + << connection->objectName(); + }); } void ConnectionEncryptionData::saveDevicesList() diff --git a/Quotient/connectionencryptiondata_p.h b/Quotient/connectionencryptiondata_p.h index 81e38d821..2152b4f78 100644 --- a/Quotient/connectionencryptiondata_p.h +++ b/Quotient/connectionencryptiondata_p.h @@ -16,8 +16,7 @@ struct DevicesList; namespace _impl { class ConnectionEncryptionData { public: - static std::optional> setup(Connection* connection, - bool mock = false); + static void setup(Connection* connection, std::function>)> then, bool mock = false); Connection* q; QOlmAccount olmAccount; From 97828df64e898d289d80d86dba8f66a43d4cc05d Mon Sep 17 00:00:00 2001 From: Tobias Fella Date: Thu, 18 Jul 2024 18:54:40 +0200 Subject: [PATCH 2/8] Add Connection::ready signal This is important for clients to know the point from which it is safe to load state without risking problems due to the e2ee machinery not being ready yet. --- Quotient/connection.cpp | 1 + Quotient/connection.h | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/Quotient/connection.cpp b/Quotient/connection.cpp index 8a5aac23a..10dd4299b 100644 --- a/Quotient/connection.cpp +++ b/Quotient/connection.cpp @@ -327,6 +327,7 @@ void Connection::Private::completeSetup(const QString& mxId, bool mock) useEncryption = false; emit q->encryptionChanged(false); } + emit q->ready(); }, mock); } else qCInfo(E2EE) << "End-to-end encryption (E2EE) support is off for" diff --git a/Quotient/connection.h b/Quotient/connection.h index b3301fc1d..156c49956 100644 --- a/Quotient/connection.h +++ b/Quotient/connection.h @@ -922,6 +922,10 @@ public Q_SLOTS: //! whether to create them now and then set them up, if desired. void crossSigningSetupRequired(); + //! The connection is ready to be used. Most notably, the fundamental e2ee data is loaded. + //! This does not mean that the server was reached, a sync was performed, or the state cache was loaded. + void ready(); + friend class ::TestCrossSigning; protected: //! Access the underlying ConnectionData class From 8d3b7a2e17aca49eedc7c43e782c81469793c910 Mon Sep 17 00:00:00 2001 From: Tobias Fella Date: Thu, 18 Jul 2024 18:56:25 +0200 Subject: [PATCH 3/8] Connection::assumeIdentity: Set UserId and DeviceId and completeSetup before connecting to the server This allows libQuotient-based clients to operate with the cached state (and in the future, room events) when there is no network connection. It's losing the check whether userid and deviceid correspond to the given token, but I don't see a strong need for that. --- Quotient/accountregistry.cpp | 1 + Quotient/connection.cpp | 6 +++--- Quotient/connection.h | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Quotient/accountregistry.cpp b/Quotient/accountregistry.cpp index 40e5e7f46..caf980856 100644 --- a/Quotient/accountregistry.cpp +++ b/Quotient/accountregistry.cpp @@ -144,6 +144,7 @@ void AccountRegistry::invokeLogin() }); connection->assumeIdentity( account.userId(), + account.deviceId(), QString::fromUtf8(accessTokenLoadingJob->binaryData())); add(connection); }); diff --git a/Quotient/connection.cpp b/Quotient/connection.cpp index 10dd4299b..99729c9e5 100644 --- a/Quotient/connection.cpp +++ b/Quotient/connection.cpp @@ -170,8 +170,10 @@ void Connection::loginWithToken(const QString& loginToken, QString() /*password*/, loginToken, deviceId, initialDeviceName); } -void Connection::assumeIdentity(const QString& mxId, const QString& accessToken) +void Connection::assumeIdentity(const QString& mxId, const QString& deviceId, const QString& accessToken) { + d->data->setDeviceId(deviceId); + d->completeSetup(mxId); d->ensureHomeserver(mxId).then([this, mxId, accessToken] { d->data->setToken(accessToken.toLatin1()); callApi().onResult([this, mxId](const GetTokenOwnerJob* job) { @@ -181,8 +183,6 @@ void Connection::assumeIdentity(const QString& mxId, const QString& accessToken) qCWarning(MAIN).nospace() << "The access_token owner (" << job->userId() << ") is different from passed MXID (" << mxId << ")!"; - d->data->setDeviceId(job->deviceId()); - d->completeSetup(job->userId()); return; case BaseJob::NetworkError: emit networkError(job->errorString(), job->rawDataSample(), job->maxRetries(), -1); diff --git a/Quotient/connection.h b/Quotient/connection.h index 156c49956..8d3c68a39 100644 --- a/Quotient/connection.h +++ b/Quotient/connection.h @@ -659,7 +659,7 @@ public Q_SLOTS: //! Similar to loginWithPassword(), this method checks that the homeserver //! URL is valid and tries to resolve it from the MXID in case it is not. //! \since 0.7.2 - void assumeIdentity(const QString& mxId, const QString& accessToken); + void assumeIdentity(const QString& mxId, const QString& deviceId, const QString& accessToken); //! \brief Request supported spec versions from the homeserver //! From 15daf963fd273cb0b2f02abd9c1aef22d2c946a5 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Wed, 24 Jul 2024 22:53:56 +0200 Subject: [PATCH 4/8] Use futures instead of std::function() callbacks --- Quotient/connection.cpp | 34 ++--- Quotient/connectionencryptiondata_p.cpp | 160 ++++++++++++------------ Quotient/connectionencryptiondata_p.h | 3 +- 3 files changed, 104 insertions(+), 93 deletions(-) diff --git a/Quotient/connection.cpp b/Quotient/connection.cpp index 99729c9e5..ebc2e4fe8 100644 --- a/Quotient/connection.cpp +++ b/Quotient/connection.cpp @@ -319,22 +319,28 @@ void Connection::Private::completeSetup(const QString& mxId, bool mock) q->user()->load(); // Load the local user's profile } + emit q->stateChanged(); // Technically connected to the homeserver but no E2EE yet + if (useEncryption) { - _impl::ConnectionEncryptionData::setup(q, [this](auto&& maybeEncryptionData){ - if (maybeEncryptionData) { - encryptionData = std::move(*maybeEncryptionData); - } else { - useEncryption = false; - emit q->encryptionChanged(false); - } - emit q->ready(); - }, mock); - } else - qCInfo(E2EE) << "End-to-end encryption (E2EE) support is off for" - << q->objectName(); + using _impl::ConnectionEncryptionData; + ConnectionEncryptionData::setup(q, mock).then( + [this](std::unique_ptr&& encData) { + if (encData) { + encryptionData = std::move(encData); + } else { + useEncryption = false; + emit q->encryptionChanged(false); + } + emit q->ready(); + emit q->stateChanged(); + emit q->connected(); + }); + } else { + qCInfo(E2EE) << "End-to-end encryption (E2EE) support is off for" << q->objectName(); + emit q->ready(); + emit q->connected(); + } - emit q->stateChanged(); - emit q->connected(); } QFuture Connection::Private::ensureHomeserver(const QString& userId, diff --git a/Quotient/connectionencryptiondata_p.cpp b/Quotient/connectionencryptiondata_p.cpp index 4aa5b4b6d..669239fa8 100644 --- a/Quotient/connectionencryptiondata_p.cpp +++ b/Quotient/connectionencryptiondata_p.cpp @@ -14,113 +14,117 @@ #include #include +#include using namespace Quotient; using namespace Quotient::_impl; -void setupPicklingKey(const QString& id, bool mock, std::function)> then) +QFuture setupPicklingKey(const QString& id, bool mock) { if (mock) { qInfo(E2EE) << "Using a mock pickling key"; - then(PicklingKey::generate()); - return; + return QtFuture::makeReadyFuture(PicklingKey::generate()); } - // TODO: Rewrite the whole thing in an async way to get rid of nested event - // loops + QPromise promise; + auto futureResult = promise.future(); + promise.start(); + using namespace QKeychain; const auto keychainId = id + "-Pickle"_ls; auto readJob = new ReadPasswordJob(qAppName()); readJob->setAutoDelete(true); - qWarning() << "app" << qAppName() << "id" << keychainId; + qCInfo(MAIN) << "Keychain request: app" << qAppName() << "id" << keychainId; readJob->setKey(keychainId); - QObject::connect(readJob, &Job::finished, readJob, [readJob, then, id, keychainId](){ - if (readJob->error() == Error::NoError) { - auto&& data = readJob->binaryData(); - if (data.size() == PicklingKey::extent) { - qDebug(E2EE) << "Successfully loaded pickling key from keychain"; - then(PicklingKey::fromByteArray(std::move(data))); + QObject::connect(readJob, &Job::finished, readJob, + [readJob, keychainId, promise = std::move(promise)]() mutable { + switch (readJob->error()) { + case Error::NoError: { + auto&& data = readJob->binaryData(); + if (data.size() == PicklingKey::extent) { + qDebug(E2EE) << "Successfully loaded pickling key from keychain"; + promise.addResult(PicklingKey::fromByteArray(std::move(data))); + } else { + qCritical(E2EE) + << "The pickling key loaded from" << keychainId << "has length" + << data.size() << "but the library expected" << PicklingKey::extent; + promise.future().cancel(); + } + promise.finish(); return; } - qCritical(E2EE) << "The loaded pickling key for" << id - << "has length" << data.size() - << "but the library expected" << PicklingKey::extent; - then(Error::OtherError); - return; - } - if (readJob->error() == Error::EntryNotFound) { - auto&& picklingKey = PicklingKey::generate(); - auto writeJob = new WritePasswordJob(qAppName()); - writeJob->setAutoDelete(true); - writeJob->setKey(keychainId); - writeJob->setBinaryData(picklingKey.viewAsByteArray()); - QObject::connect(writeJob, &Job::finished, writeJob, [writeJob, then, &picklingKey](){ + case Error::EntryNotFound: { + auto&& picklingKey = PicklingKey::generate(); + auto writeJob = new WritePasswordJob(qAppName()); + writeJob->setAutoDelete(true); + writeJob->setKey(keychainId); + writeJob->setBinaryData(picklingKey.viewAsByteArray()); + qDebug(E2EE) << "Saving a new pickling key to the keychain"; + promise.addResult(std::move(picklingKey)); // but don't finish yet + QObject::connect(writeJob, &Job::finished, writeJob, + [promise = std::move(promise), writeJob]() mutable { + if (writeJob->error() != Error::NoError) { + qCritical(E2EE) + << "Could not save pickling key to keychain: " + << writeJob->errorString(); + promise.future().cancel(); + } else + promise.finish(); + }); writeJob->start(); - - if (writeJob->error() == Error::NoError) { - then(std::move(picklingKey)); - return; - } - - qCritical(E2EE) << "Could not save pickling key to keychain: " - << writeJob->errorString(); - then(writeJob->error()); - }); - } - qWarning(E2EE) << "Error loading pickling key - please fix your keychain:" - << readJob->errorString(); - then(readJob->error()); - return; - }); + return; + } + default: + qWarning(E2EE) << "Error loading pickling key - please fix your keychain:" + << readJob->errorString(); + promise.future().cancel(); + } + }); readJob->start(); + return futureResult; } -void ConnectionEncryptionData::setup(Connection* connection, std::function>)> then, bool mock) +QFuture> ConnectionEncryptionData::setup( + Connection* connection, bool mock) { - setupPicklingKey(connection->userId(), mock, [connection, then, mock](Expected maybePicklingKey){ - if(maybePicklingKey) { - auto&& encryptionData = std::make_unique( - connection, std::move(*maybePicklingKey)); + return setupPicklingKey(connection->userId(), mock) + .then([connection, mock](QFuture ft) { + auto encryptionData = + std::make_unique(connection, ft.takeResult()); if (mock) { encryptionData->database.clear(); encryptionData->olmAccount.setupNewAccount(); - then(std::move(encryptionData)); - return; + return encryptionData; } - if (const auto outcome = encryptionData->database.setupOlmAccount( - encryptionData->olmAccount)) { + if (const auto outcome = + encryptionData->database.setupOlmAccount(encryptionData->olmAccount)) { // account already existing or there's an error unpickling it - if (outcome == OLM_SUCCESS) { - then(std::move(encryptionData)); - return; - } + if (outcome == OLM_SUCCESS) + return encryptionData; - qCritical(E2EE) << "Could not unpickle Olm account for" - << connection->objectName(); - then(std::nullopt); - return; + qCritical(E2EE) << "Could not unpickle Olm account for" << connection->objectName(); + ft.cancel(); + return std::unique_ptr{}; } // A new account has been created - auto job = connection->callApi( - encryptionData->olmAccount.deviceKeys()); - // eData is meant to have the same scope as connection so it's safe - // to pass an unguarded pointer to encryption data here - QObject::connect(job, &BaseJob::success, connection, - [connection, eData = encryptionData.get()] { - eData->trackedUsers += connection->userId(); - eData->outdatedUsers += connection->userId(); - eData->encryptionUpdateRequired = true; - }); - QObject::connect(job, &BaseJob::failure, connection, [job] { - qCWarning(E2EE) - << "Failed to upload device keys:" << job->errorString(); - }); - then(std::move(encryptionData)); - return; - } - qCritical(E2EE) << "Could not load or initialise a pickling key for" - << connection->objectName(); - }); + connection->callApi(encryptionData->olmAccount.deviceKeys()) + .then(connection, + // eData is meant to have the same lifetime as connection so it's safe + // to pass an unguarded pointer to encryption data here + [connection, eData = encryptionData.get()] { + eData->trackedUsers += connection->userId(); + eData->outdatedUsers += connection->userId(); + eData->encryptionUpdateRequired = true; + }, + [](auto* job) { + qCWarning(E2EE) << "Failed to upload device keys:" << job->errorString(); + }); + return encryptionData; + }) + .onCanceled([connection] { + qCritical(E2EE) << "Could not setup E2EE for" << connection->objectName(); + return std::unique_ptr{}; + }); } void ConnectionEncryptionData::saveDevicesList() diff --git a/Quotient/connectionencryptiondata_p.h b/Quotient/connectionencryptiondata_p.h index 2152b4f78..ab51e0b9d 100644 --- a/Quotient/connectionencryptiondata_p.h +++ b/Quotient/connectionencryptiondata_p.h @@ -16,7 +16,8 @@ struct DevicesList; namespace _impl { class ConnectionEncryptionData { public: - static void setup(Connection* connection, std::function>)> then, bool mock = false); + static QFuture> setup(Connection* connection, + bool mock = false); Connection* q; QOlmAccount olmAccount; From 0493ba519108847caef34a4fb9c0642ec576eaec Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Thu, 25 Jul 2024 09:08:29 +0200 Subject: [PATCH 5/8] CI: Bump Qt Keychain to 0.14.3 Linux pipelines fail mysteriously since the rewrite, with the keychain job getting deleted before yielding the result - maybe the newest Qt Keychain would be better? --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ae568cb6a..a6cf15f75 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -137,7 +137,7 @@ jobs: - name: Build and install QtKeychain run: | cd .. - git clone -b v0.13.2 https://github.com/frankosterfeld/qtkeychain.git + git clone -b v0.14.3 https://github.com/frankosterfeld/qtkeychain.git cmake -S qtkeychain -B qtkeychain/build -DBUILD_WITH_QT6=ON $CMAKE_ARGS cmake --build qtkeychain/build --target install From e3fbbc5d267609ec56df21f535205cb1826f85db Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Thu, 25 Jul 2024 09:36:40 +0200 Subject: [PATCH 6/8] CI: Now use the correct tag for Qt Keychain --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a6cf15f75..ea98d7a8d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -137,7 +137,7 @@ jobs: - name: Build and install QtKeychain run: | cd .. - git clone -b v0.14.3 https://github.com/frankosterfeld/qtkeychain.git + git clone -b 0.14.3 https://github.com/frankosterfeld/qtkeychain.git cmake -S qtkeychain -B qtkeychain/build -DBUILD_WITH_QT6=ON $CMAKE_ARGS cmake --build qtkeychain/build --target install From 9b70aa192876e91b17d6a409960bb944b268b079 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Thu, 22 Aug 2024 09:26:38 +0200 Subject: [PATCH 7/8] Replace code comments with logging --- Quotient/connectionencryptiondata_p.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Quotient/connectionencryptiondata_p.cpp b/Quotient/connectionencryptiondata_p.cpp index 669239fa8..b1b15f25f 100644 --- a/Quotient/connectionencryptiondata_p.cpp +++ b/Quotient/connectionencryptiondata_p.cpp @@ -98,15 +98,16 @@ QFuture> ConnectionEncryptionData::set } if (const auto outcome = encryptionData->database.setupOlmAccount(encryptionData->olmAccount)) { - // account already existing or there's an error unpickling it - if (outcome == OLM_SUCCESS) + if (outcome == OLM_SUCCESS) { + qCDebug(E2EE) << "The existing Olm account successfully unpickled"; return encryptionData; + } qCritical(E2EE) << "Could not unpickle Olm account for" << connection->objectName(); ft.cancel(); return std::unique_ptr{}; } - // A new account has been created + qCDebug(E2EE) << "A new Olm account has been created, uploading device keys"; connection->callApi(encryptionData->olmAccount.deviceKeys()) .then(connection, // eData is meant to have the same lifetime as connection so it's safe From e447f4165eb42128097bc26bfe7f7838d759028d Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Fri, 23 Aug 2024 18:14:26 +0200 Subject: [PATCH 8/8] Fix failing tests on Linux with Qt 6.4 Qt 6.4 doesn't handle QFutures with move-only results well. The workaround for now is to pass the reference to the ultimate move-only result (i.e. Connection::Private::encryptionData) into all the continuations as a captured variable. Once we require at least Qt 6.5 this workaround can be removed and normal QFuture logic used to pass defered results around. --- Quotient/connection.cpp | 21 ++-- Quotient/connectionencryptiondata_p.cpp | 128 ++++++++++++------------ Quotient/connectionencryptiondata_p.h | 4 +- 3 files changed, 77 insertions(+), 76 deletions(-) diff --git a/Quotient/connection.cpp b/Quotient/connection.cpp index ebc2e4fe8..a1652be1f 100644 --- a/Quotient/connection.cpp +++ b/Quotient/connection.cpp @@ -323,18 +323,15 @@ void Connection::Private::completeSetup(const QString& mxId, bool mock) if (useEncryption) { using _impl::ConnectionEncryptionData; - ConnectionEncryptionData::setup(q, mock).then( - [this](std::unique_ptr&& encData) { - if (encData) { - encryptionData = std::move(encData); - } else { - useEncryption = false; - emit q->encryptionChanged(false); - } - emit q->ready(); - emit q->stateChanged(); - emit q->connected(); - }); + ConnectionEncryptionData::setup(q, mock, encryptionData).then([this](bool successful) { + if (!successful || !encryptionData) + useEncryption = false; + + emit q->encryptionChanged(useEncryption); + emit q->stateChanged(); + emit q->ready(); + emit q->connected(); + }); } else { qCInfo(E2EE) << "End-to-end encryption (E2EE) support is off for" << q->objectName(); emit q->ready(); diff --git a/Quotient/connectionencryptiondata_p.cpp b/Quotient/connectionencryptiondata_p.cpp index b1b15f25f..53ecfec50 100644 --- a/Quotient/connectionencryptiondata_p.cpp +++ b/Quotient/connectionencryptiondata_p.cpp @@ -19,112 +19,116 @@ using namespace Quotient; using namespace Quotient::_impl; -QFuture setupPicklingKey(const QString& id, bool mock) +// Below, encryptionData gets filled inside setupPicklingKey() instead of returning the future for +// a pickling key and then, in CED::setup(), another future for ConnectionEncryptionData because +// Qt versions before 6.5.2 don't handle QFutures with move-only data quite well (see QTBUG-112513). +// Oh, and unwrap() doesn't work with move-only types at all (QTBUG-127423). So it is a bit more +// verbose and repetitive than it should be. + +inline QFuture runKeychainJob(QKeychain::Job* j, const QString& keychainId) +{ + j->setAutoDelete(true); + j->setKey(keychainId); + auto ft = QtFuture::connect(j, &QKeychain::Job::finished); + j->start(); + return ft; +} + +QFuture setupPicklingKey(Connection* connection, bool mock, + std::unique_ptr& encryptionData) { if (mock) { qInfo(E2EE) << "Using a mock pickling key"; - return QtFuture::makeReadyFuture(PicklingKey::generate()); + encryptionData = + std::make_unique(connection, PicklingKey::generate()); + return QtFuture::makeReadyFuture(); } - QPromise promise; - auto futureResult = promise.future(); - promise.start(); - using namespace QKeychain; - const auto keychainId = id + "-Pickle"_ls; - auto readJob = new ReadPasswordJob(qAppName()); - readJob->setAutoDelete(true); + const auto keychainId = connection->userId() + "-Pickle"_ls; qCInfo(MAIN) << "Keychain request: app" << qAppName() << "id" << keychainId; - readJob->setKey(keychainId); - QObject::connect(readJob, &Job::finished, readJob, - [readJob, keychainId, promise = std::move(promise)]() mutable { - switch (readJob->error()) { + + return runKeychainJob(new ReadPasswordJob(qAppName()), keychainId) + .then([keychainId, &encryptionData, connection](const Job* j) -> QFuture { + // The future will hold nullptr if the existing pickling key was found and no write is + // pending; a pointer to the write job if if a new key was made and is being written; + // be cancelled in case of an error. + switch (const auto readJob = static_cast(j); readJob->error()) { case Error::NoError: { auto&& data = readJob->binaryData(); if (data.size() == PicklingKey::extent) { qDebug(E2EE) << "Successfully loaded pickling key from keychain"; - promise.addResult(PicklingKey::fromByteArray(std::move(data))); - } else { - qCritical(E2EE) - << "The pickling key loaded from" << keychainId << "has length" - << data.size() << "but the library expected" << PicklingKey::extent; - promise.future().cancel(); + encryptionData = std::make_unique( + connection, PicklingKey::fromByteArray(std::move(data))); + return QtFuture::makeReadyFuture(nullptr); } - promise.finish(); - return; + qCritical(E2EE) + << "The pickling key loaded from" << keychainId << "has length" + << data.size() << "but the library expected" << PicklingKey::extent; + return {}; } case Error::EntryNotFound: { auto&& picklingKey = PicklingKey::generate(); auto writeJob = new WritePasswordJob(qAppName()); - writeJob->setAutoDelete(true); - writeJob->setKey(keychainId); writeJob->setBinaryData(picklingKey.viewAsByteArray()); + encryptionData = std::make_unique( + connection, std::move(picklingKey)); // the future may still get cancelled qDebug(E2EE) << "Saving a new pickling key to the keychain"; - promise.addResult(std::move(picklingKey)); // but don't finish yet - QObject::connect(writeJob, &Job::finished, writeJob, - [promise = std::move(promise), writeJob]() mutable { - if (writeJob->error() != Error::NoError) { - qCritical(E2EE) - << "Could not save pickling key to keychain: " - << writeJob->errorString(); - promise.future().cancel(); - } else - promise.finish(); - }); - writeJob->start(); - return; + return runKeychainJob(writeJob, keychainId); } default: qWarning(E2EE) << "Error loading pickling key - please fix your keychain:" << readJob->errorString(); - promise.future().cancel(); + } + return {}; + }) + .unwrap() + .then([](QFuture writeFuture) { + if (const Job* const writeJob = writeFuture.result(); + writeJob && writeJob->error() != Error::NoError) // + { + qCritical(E2EE) << "Could not save pickling key to keychain: " + << writeJob->errorString(); + writeFuture.cancel(); } }); - readJob->start(); - return futureResult; } -QFuture> ConnectionEncryptionData::setup( - Connection* connection, bool mock) +QFuture ConnectionEncryptionData::setup(Connection* connection, bool mock, + std::unique_ptr& result) { - return setupPicklingKey(connection->userId(), mock) - .then([connection, mock](QFuture ft) { - auto encryptionData = - std::make_unique(connection, ft.takeResult()); + return setupPicklingKey(connection, mock, result) + .then([connection, mock, &result] { if (mock) { - encryptionData->database.clear(); - encryptionData->olmAccount.setupNewAccount(); - return encryptionData; + result->database.clear(); + result->olmAccount.setupNewAccount(); + return true; } - if (const auto outcome = - encryptionData->database.setupOlmAccount(encryptionData->olmAccount)) { + if (const auto outcome = result->database.setupOlmAccount(result->olmAccount)) { if (outcome == OLM_SUCCESS) { qCDebug(E2EE) << "The existing Olm account successfully unpickled"; - return encryptionData; + return true; } qCritical(E2EE) << "Could not unpickle Olm account for" << connection->objectName(); - ft.cancel(); - return std::unique_ptr{}; + return false; } qCDebug(E2EE) << "A new Olm account has been created, uploading device keys"; - connection->callApi(encryptionData->olmAccount.deviceKeys()) + connection->callApi(result->olmAccount.deviceKeys()) .then(connection, - // eData is meant to have the same lifetime as connection so it's safe - // to pass an unguarded pointer to encryption data here - [connection, eData = encryptionData.get()] { - eData->trackedUsers += connection->userId(); - eData->outdatedUsers += connection->userId(); - eData->encryptionUpdateRequired = true; + [connection, &result] { + result->trackedUsers += connection->userId(); + result->outdatedUsers += connection->userId(); + result->encryptionUpdateRequired = true; }, [](auto* job) { qCWarning(E2EE) << "Failed to upload device keys:" << job->errorString(); }); - return encryptionData; + return true; }) .onCanceled([connection] { qCritical(E2EE) << "Could not setup E2EE for" << connection->objectName(); - return std::unique_ptr{}; + return false; }); } diff --git a/Quotient/connectionencryptiondata_p.h b/Quotient/connectionencryptiondata_p.h index ab51e0b9d..970dbcbb8 100644 --- a/Quotient/connectionencryptiondata_p.h +++ b/Quotient/connectionencryptiondata_p.h @@ -16,8 +16,8 @@ struct DevicesList; namespace _impl { class ConnectionEncryptionData { public: - static QFuture> setup(Connection* connection, - bool mock = false); + static QFuture setup(Connection* connection, bool mock, + std::unique_ptr& result); Connection* q; QOlmAccount olmAccount;