From 4f07a6592c7b098e2a8caa6dd61f2f70d8e19b03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sami=20V=C3=A4nttinen?= Date: Wed, 26 Oct 2022 10:24:49 +0300 Subject: [PATCH] Revert async Access Confirm Dialog --- src/browser/BrowserAccessControlDialog.cpp | 52 +---- src/browser/BrowserAccessControlDialog.h | 16 +- src/browser/BrowserAction.cpp | 36 ++-- src/browser/BrowserAction.h | 2 +- src/browser/BrowserService.cpp | 229 +++++++-------------- src/browser/BrowserService.h | 53 ++--- 6 files changed, 102 insertions(+), 286 deletions(-) diff --git a/src/browser/BrowserAccessControlDialog.cpp b/src/browser/BrowserAccessControlDialog.cpp index f55ea8b83..3fce10c5c 100644 --- a/src/browser/BrowserAccessControlDialog.cpp +++ b/src/browser/BrowserAccessControlDialog.cpp @@ -32,30 +32,16 @@ BrowserAccessControlDialog::BrowserAccessControlDialog(QWidget* parent) m_ui->setupUi(this); - connect(m_ui->allowButton, SIGNAL(clicked()), SLOT(acceptSelections())); - connect(m_ui->denyButton, SIGNAL(clicked()), SLOT(rejectSelections())); - connect(this, SIGNAL(rejected()), this, SIGNAL(closed())); + connect(m_ui->allowButton, SIGNAL(clicked()), SLOT(accept())); + connect(m_ui->denyButton, SIGNAL(clicked()), SLOT(reject())); } BrowserAccessControlDialog::~BrowserAccessControlDialog() { } -void BrowserAccessControlDialog::closeEvent(QCloseEvent* event) +void BrowserAccessControlDialog::setItems(const QList& items, const QString& urlString, bool httpAuth) { - // Emits closed signal when clicking X from title bar - emit closed(); - event->accept(); -} - -void BrowserAccessControlDialog::setItems(const QList& entriesToConfirm, - const QList& allowedEntries, - const QString& urlString, - bool httpAuth) -{ - m_entriesToConfirm = entriesToConfirm; - m_allowedEntries = allowedEntries; - QUrl url(urlString); m_ui->siteLabel->setText(m_ui->siteLabel->text().arg( url.toDisplayString(QUrl::RemoveUserInfo | QUrl::RemovePath | QUrl::RemoveQuery | QUrl::RemoveFragment))); @@ -63,11 +49,11 @@ void BrowserAccessControlDialog::setItems(const QList& entriesToConfirm, m_ui->rememberDecisionCheckBox->setVisible(!httpAuth); m_ui->rememberDecisionCheckBox->setChecked(false); - m_ui->itemsTable->setRowCount(entriesToConfirm.count()); + m_ui->itemsTable->setRowCount(items.count()); m_ui->itemsTable->setColumnCount(2); int row = 0; - for (const auto& entry : entriesToConfirm) { + for (const auto& entry : items) { auto item = new QTableWidgetItem(); item->setText(entry->title() + " - " + entry->username()); item->setData(Qt::UserRole, row); @@ -77,23 +63,18 @@ void BrowserAccessControlDialog::setItems(const QList& entriesToConfirm, auto disableButton = new QPushButton(tr("Disable for this site")); disableButton->setAutoDefault(false); - connect(disableButton, &QAbstractButton::pressed, [&, item] { emit disableAccess(item); m_ui->itemsTable->removeRow(item->row()); - if (m_ui->itemsTable->rowCount() == 0) { - emit closed(); + reject(); } }); m_ui->itemsTable->setCellWidget(row, 1, disableButton); - ++row; } - m_ui->itemsTable->resizeColumnsToContents(); m_ui->itemsTable->horizontalHeader()->setSectionResizeMode(0, QHeaderView::Stretch); - m_ui->allowButton->setFocus(); } @@ -102,11 +83,6 @@ bool BrowserAccessControlDialog::remember() const return m_ui->rememberDecisionCheckBox->isChecked(); } -bool BrowserAccessControlDialog::entriesAccepted() const -{ - return m_entriesAccepted; -} - QList BrowserAccessControlDialog::getSelectedEntries() const { QList selected; @@ -130,19 +106,3 @@ QList BrowserAccessControlDialog::getNonSelectedEntries() con } return notSelected; } - -void BrowserAccessControlDialog::acceptSelections() -{ - auto selectedEntries = getSelectedEntries(); - - m_entriesAccepted = true; - emit acceptEntries(selectedEntries, m_entriesToConfirm, m_allowedEntries); - emit closed(); -} - -void BrowserAccessControlDialog::rejectSelections() -{ - auto rejectedEntries = getNonSelectedEntries(); - emit rejectEntries(rejectedEntries, m_entriesToConfirm); - emit closed(); -} diff --git a/src/browser/BrowserAccessControlDialog.h b/src/browser/BrowserAccessControlDialog.h index c41c6c77e..28f75303b 100644 --- a/src/browser/BrowserAccessControlDialog.h +++ b/src/browser/BrowserAccessControlDialog.h @@ -37,28 +37,14 @@ public: explicit BrowserAccessControlDialog(QWidget* parent = nullptr); ~BrowserAccessControlDialog() override; - void setItems(const QList& entriesToConfirm, - const QList& allowedEntries, - const QString& urlString, - bool httpAuth); + void setItems(const QList& items, const QString& urlString, bool httpAuth); bool remember() const; - bool entriesAccepted() const; QList getSelectedEntries() const; QList getNonSelectedEntries() const; signals: void disableAccess(QTableWidgetItem* item); - void acceptEntries(QList items, QList entriesToConfirm, QList allowedEntries); - void rejectEntries(QList items, QList entriesToConfirm); - void closed(); - -public slots: - void acceptSelections(); - void rejectSelections(); - -private: - void closeEvent(QCloseEvent* event) override; private: QScopedPointer m_ui; diff --git a/src/browser/BrowserAction.cpp b/src/browser/BrowserAction.cpp index 1d789d470..9e555a43f 100644 --- a/src/browser/BrowserAction.cpp +++ b/src/browser/BrowserAction.cpp @@ -76,7 +76,7 @@ QJsonObject BrowserAction::handleAction(QLocalSocket* socket, const QJsonObject& } else if (action.compare("test-associate") == 0) { return handleTestAssociate(json, action); } else if (action.compare("get-logins") == 0) { - return handleGetLogins(socket, json, action); + return handleGetLogins(json, action); } else if (action.compare("generate-password") == 0) { return handleGeneratePassword(socket, json, action); } else if (action.compare("set-login") == 0) { @@ -231,7 +231,7 @@ QJsonObject BrowserAction::handleTestAssociate(const QJsonObject& json, const QS return buildResponse(action, message, newNonce); } -QJsonObject BrowserAction::handleGetLogins(QLocalSocket* socket, const QJsonObject& json, const QString& action) +QJsonObject BrowserAction::handleGetLogins(const QJsonObject& json, const QString& action) { const QString hash = browserService()->getDatabaseHash(); const QString nonce = json.value("nonce").toString(); @@ -264,31 +264,19 @@ QJsonObject BrowserAction::handleGetLogins(QLocalSocket* socket, const QJsonObje const QString formUrl = decrypted.value("submitUrl").toString(); const QString auth = decrypted.value("httpAuth").toString(); const bool httpAuth = auth.compare(TRUE_STR) == 0; - auto requestId = decrypted.value("requestID").toString(); - if (browserService()->isAccessConfirmRequested()) { - auto errorReply = getErrorReply(action, ERROR_KEEPASS_ACTION_CANCELLED_OR_DENIED); - - if (!requestId.isEmpty()) { - errorReply["requestID"] = requestId; - } - - return errorReply; + const QJsonArray users = browserService()->findMatchingEntries(id, siteUrl, formUrl, "", keyList, httpAuth); + if (users.isEmpty()) { + return getErrorReply(action, ERROR_KEEPASS_NO_LOGINS_FOUND); } - browserService()->findEntries(socket, - incrementedNonce, - m_clientPublicKey, - m_secretKey, - id, - hash, - requestId, - siteUrl, - formUrl, - "", - keyList, - httpAuth); - return QJsonObject(); + QJsonObject message = browserMessageBuilder()->buildMessage(incrementedNonce); + message["count"] = users.count(); + message["entries"] = users; + message["hash"] = hash; + message["id"] = id; + + return buildResponse(action, message, incrementedNonce); } QJsonObject BrowserAction::handleGeneratePassword(QLocalSocket* socket, const QJsonObject& json, const QString& action) diff --git a/src/browser/BrowserAction.h b/src/browser/BrowserAction.h index 6f0b9a6cf..49c66b644 100644 --- a/src/browser/BrowserAction.h +++ b/src/browser/BrowserAction.h @@ -37,7 +37,7 @@ private: QJsonObject handleGetDatabaseHash(const QJsonObject& json, const QString& action); QJsonObject handleAssociate(const QJsonObject& json, const QString& action); QJsonObject handleTestAssociate(const QJsonObject& json, const QString& action); - QJsonObject handleGetLogins(QLocalSocket* socket, const QJsonObject& json, const QString& action); + QJsonObject handleGetLogins(const QJsonObject& json, const QString& action); QJsonObject handleGeneratePassword(QLocalSocket* socket, const QJsonObject& json, const QString& action); QJsonObject handleSetLogin(const QJsonObject& json, const QString& action); QJsonObject handleLockDatabase(const QJsonObject& json, const QString& action); diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index cee32bb08..2953cf582 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -1,6 +1,5 @@ /* * Copyright (C) 2013 Francois Ferrand - * Copyright (C) 2017 Sami Vänttinen * Copyright (C) 2022 KeePassXC Team * * This program is free software: you can redistribute it and/or modify @@ -64,9 +63,9 @@ Q_GLOBAL_STATIC(BrowserService, s_browserService); BrowserService::BrowserService() : QObject() , m_browserHost(new BrowserHost) + , m_dialogActive(false) , m_bringToFrontRequested(false) , m_passwordGeneratorRequested(false) - , m_accessConfirmRequested(false) , m_prevWindowState(WindowState::Normal) , m_keepassBrowserUUID(Tools::hexToUuid("de887cc3036343b8974b5911b8816224")) { @@ -314,18 +313,12 @@ QString BrowserService::getCurrentTotp(const QString& uuid) return {}; } -void BrowserService::findEntries(QLocalSocket* socket, - const QString& nonce, - const QString& publicKey, - const QString& secretKey, - const QString& dbid, - const QString& hash, - const QString& requestId, - const QString& siteUrl, - const QString& formUrl, - const QString& realm, - const StringPairList& keyList, - const bool httpAuth) +QJsonArray BrowserService::findMatchingEntries(const QString& dbid, + const QString& siteUrl, + const QString& formUrl, + const QString& realm, + const StringPairList& keyList, + const bool httpAuth) { Q_UNUSED(dbid); const bool alwaysAllowAccess = browserSettings()->alwaysAllowAccess(); @@ -334,8 +327,8 @@ void BrowserService::findEntries(QLocalSocket* socket, const QString formHost = QUrl(formUrl).host(); // Check entries for authorization - QList entriesToConfirm; - QList allowedEntries; + QList pwEntriesToConfirm; + QList pwEntries; for (auto* entry : searchEntries(siteUrl, formUrl, keyList)) { auto entryCustomData = entry->customData(); @@ -355,7 +348,7 @@ void BrowserService::findEntries(QLocalSocket* socket, // HTTP Basic Auth always needs a confirmation if (!ignoreHttpAuth && httpAuth) { - entriesToConfirm.append(entry); + pwEntriesToConfirm.append(entry); continue; } @@ -365,166 +358,87 @@ void BrowserService::findEntries(QLocalSocket* socket, case Unknown: if (alwaysAllowAccess) { - allowedEntries.append(entry); + pwEntries.append(entry); } else { - entriesToConfirm.append(entry); + pwEntriesToConfirm.append(entry); } break; case Allowed: - allowedEntries.append(entry); + pwEntries.append(entry); break; } } - if (entriesToConfirm.isEmpty()) { - sendCredentialsToClient(allowedEntries, socket, nonce, publicKey, secretKey, hash, dbid, siteUrl, formUrl); - return; + // Confirm entries + QList selectedEntriesToConfirm = + confirmEntries(pwEntriesToConfirm, siteUrl, siteHost, formHost, realm, httpAuth); + if (!selectedEntriesToConfirm.isEmpty()) { + pwEntries.append(selectedEntriesToConfirm); } - confirmEntries(socket, - nonce, - publicKey, - secretKey, - dbid, - hash, - requestId, - allowedEntries, - entriesToConfirm, - siteUrl, - siteHost, - formHost, - realm, - httpAuth); -} - -void BrowserService::confirmEntries(QLocalSocket* socket, - const QString& incrementedNonce, - const QString& publicKey, - const QString& secretKey, - const QString& id, - const QString& hash, - const QString& requestId, - QList& allowedEntries, - QList& entriesToConfirm, - const QString& siteUrl, - const QString& siteHost, - const QString& formUrl, - const QString& realm, - const bool httpAuth) -{ - if (entriesToConfirm.isEmpty() || m_accessConfirmRequested) { - return; + if (pwEntries.isEmpty()) { + return {}; } - if (!m_accessControlDialog) { - - m_accessControlDialog.reset(new BrowserAccessControlDialog()); - - connect( - m_currentDatabaseWidget, SIGNAL(databaseLockRequested()), m_accessControlDialog.data(), SIGNAL(closed())); - - connect(m_accessControlDialog.data(), - &BrowserAccessControlDialog::disableAccess, - m_accessControlDialog.data(), - [=](QTableWidgetItem* item) { - auto entry = entriesToConfirm[item->row()]; - denyEntry(entry, siteHost, formUrl, realm); - }); - - connect(m_accessControlDialog.data(), &BrowserAccessControlDialog::closed, m_accessControlDialog.data(), [=] { - if (!m_accessControlDialog->entriesAccepted()) { - auto errorMessage = - browserMessageBuilder()->getErrorReply("get-logins", ERROR_KEEPASS_ACTION_CANCELLED_OR_DENIED); - errorMessage["requestID"] = requestId; - m_browserHost->sendClientMessage(socket, errorMessage); - } - - m_accessControlDialog.reset(); - hideWindow(); - m_accessConfirmRequested = false; - }); - - connect(m_accessControlDialog.data(), - &BrowserAccessControlDialog::acceptEntries, - m_accessControlDialog.data(), - [=](QList items, QList entries, QList allowed) { - QList selectedEntries; - - for (auto item : items) { - auto entry = entries[item->row()]; - if (m_accessControlDialog->remember()) { - allowEntry(entry, siteHost, formUrl, realm); - } - - selectedEntries.append(entry); - } - - hideWindow(); - - if (!selectedEntries.isEmpty()) { - allowed.append(selectedEntries); - } - - if (allowed.isEmpty()) { - return; - } - - // Ensure that database is not locked when the popup was visible - if (!isDatabaseOpened()) { - return; - } - - sendCredentialsToClient( - allowed, socket, incrementedNonce, publicKey, secretKey, hash, id, siteUrl, formUrl); - }); - - connect(m_accessControlDialog.data(), - &BrowserAccessControlDialog::rejectEntries, - m_accessControlDialog.data(), - [=](QList items, QList entries) { - Q_UNUSED(items); // We might need this later if single entries can be denied from the list - for (auto entry : entries) { - if (m_accessControlDialog->remember()) { - denyEntry(entry, siteHost, formUrl, realm); - } - } - }); - - m_accessControlDialog->setItems(entriesToConfirm, allowedEntries, siteUrl, httpAuth); - m_accessControlDialog->show(); + // Ensure that database is not locked when the popup was visible + if (!isDatabaseOpened()) { + return {}; } - m_accessConfirmRequested = true; - updateWindowState(); -} - -void BrowserService::sendCredentialsToClient(QList& allowedEntries, - QLocalSocket* socket, - const QString& incrementedNonce, - const QString& publicKey, - const QString& secretKey, - const QString& hash, - const QString& id, - const QString siteUrl, - const QString& formUrl) -{ - allowedEntries = sortEntries(allowedEntries, siteUrl, formUrl); + // Sort results + pwEntries = sortEntries(pwEntries, siteUrl, formUrl); + // Fill the list QJsonArray result; - for (auto* entry : allowedEntries) { + for (auto* entry : pwEntries) { result.append(prepareEntry(entry)); } - QJsonObject message = browserMessageBuilder()->buildMessage(incrementedNonce); - message["count"] = result.count(); - message["entries"] = result; - message["hash"] = hash; - message["id"] = id; + return result; +} - m_browserHost->sendClientMessage( - socket, browserMessageBuilder()->buildResponse("get-logins", message, incrementedNonce, publicKey, secretKey)); +QList BrowserService::confirmEntries(QList& pwEntriesToConfirm, + const QString& siteUrl, + const QString& siteHost, + const QString& formUrl, + const QString& realm, + const bool httpAuth) +{ + if (pwEntriesToConfirm.isEmpty() || m_dialogActive) { + return {}; + } + + m_dialogActive = true; + updateWindowState(); + BrowserAccessControlDialog accessControlDialog; + + connect(m_currentDatabaseWidget, SIGNAL(databaseLockRequested()), &accessControlDialog, SLOT(reject())); + + connect(&accessControlDialog, &BrowserAccessControlDialog::disableAccess, [&](QTableWidgetItem* item) { + auto entry = pwEntriesToConfirm[item->row()]; + denyEntry(entry, siteHost, formUrl, realm); + }); + + accessControlDialog.setItems(pwEntriesToConfirm, siteUrl, httpAuth); + + QList allowedEntries; + auto ret = accessControlDialog.exec(); + if (ret == QDialog::Accepted) { + for (auto item : accessControlDialog.getSelectedEntries()) { + auto entry = pwEntriesToConfirm[item->row()]; + if (accessControlDialog.remember()) { + allowEntry(entry, siteHost, formUrl, realm); + } + allowedEntries.append(entry); + } + } + + // Re-hide the application if it wasn't visible before hideWindow(); + m_dialogActive = false; + + return allowedEntries; } void BrowserService::showPasswordGenerator(QLocalSocket* socket, @@ -572,11 +486,6 @@ bool BrowserService::isPasswordGeneratorRequested() const return m_passwordGeneratorRequested; } -bool BrowserService::isAccessConfirmRequested() const -{ - return m_accessConfirmRequested; -} - QString BrowserService::storeKey(const QString& key) { auto db = getDatabase(); diff --git a/src/browser/BrowserService.h b/src/browser/BrowserService.h index 27b977257..b9e153248 100644 --- a/src/browser/BrowserService.h +++ b/src/browser/BrowserService.h @@ -1,6 +1,5 @@ /* * Copyright (C) 2013 Francois Ferrand - * Copyright (C) 2017 Sami Vänttinen * Copyright (C) 2022 KeePassXC Team * * This program is free software: you can redistribute it and/or modify @@ -63,9 +62,7 @@ public: const QString& nonce, const QString& publicKey, const QString& secretKey); - void sendPassword(QLocalSocket* socket, const QJsonObject& message); bool isPasswordGeneratorRequested() const; - bool isAccessConfirmRequested() const; void addEntry(const QString& dbid, const QString& login, @@ -84,18 +81,12 @@ public: const QString& siteUrl, const QString& formUrl); bool deleteEntry(const QString& uuid); - void findEntries(QLocalSocket* socket, - const QString& nonce, - const QString& publicKey, - const QString& secretKey, - const QString& dbid, - const QString& hash, - const QString& requestId, - const QString& siteUrl, - const QString& formUrl, - const QString& realm, - const StringPairList& keyList, - const bool httpAuth = false); + QJsonArray findMatchingEntries(const QString& dbid, + const QString& siteUrlStr, + const QString& formUrlStr, + const QString& realm, + const StringPairList& keyList, + const bool httpAuth = false); void requestGlobalAutoType(const QString& search); static void convertAttributesToCustomData(QSharedPointer db); @@ -138,29 +129,12 @@ private: QList searchEntries(const QSharedPointer& db, const QString& siteUrl, const QString& formUrl); QList searchEntries(const QString& siteUrl, const QString& formUrl, const StringPairList& keyList); QList sortEntries(QList& pwEntries, const QString& siteUrl, const QString& formUrl); - void confirmEntries(QLocalSocket* socket, - const QString& incrementedNonce, - const QString& publicKey, - const QString& secretKey, - const QString& id, - const QString& hash, - const QString& requestId, - QList& allowedEntries, - QList& entriesToConfirm, - const QString& siteUrl, - const QString& siteHost, - const QString& formUrl, - const QString& realm, - const bool httpAuth); - void sendCredentialsToClient(QList& allowedEntries, - QLocalSocket* socket, - const QString& incrementedNonce, - const QString& publicKey, - const QString& secretKey, - const QString& hash, - const QString& id, - const QString siteUrl, - const QString& formUrl); + QList confirmEntries(QList& pwEntriesToConfirm, + const QString& siteUrl, + const QString& siteHost, + const QString& formUrl, + const QString& realm, + const bool httpAuth); QJsonObject prepareEntry(const Entry* entry); void allowEntry(Entry* entry, const QString& siteHost, const QString& formUrl, const QString& realm); void denyEntry(Entry* entry, const QString& siteHost, const QString& formUrl, const QString& realm); @@ -196,15 +170,14 @@ private: QPointer m_browserHost; QHash> m_browserClients; + bool m_dialogActive; bool m_bringToFrontRequested; bool m_passwordGeneratorRequested; - bool m_accessConfirmRequested; WindowState m_prevWindowState; QUuid m_keepassBrowserUUID; QPointer m_currentDatabaseWidget; QScopedPointer m_passwordGenerator; - QScopedPointer m_accessControlDialog; Q_DISABLE_COPY(BrowserService);