From 1b12c958c5f40a345dae1d5416f3756b52fb1680 Mon Sep 17 00:00:00 2001 From: Blessio <24902257+blessio@users.noreply.github.com> Date: Sun, 6 Aug 2023 03:15:22 +0200 Subject: [PATCH] Improve error message when browser proxy cannot be found (#9385) Co-authored-by: Blessio Co-authored-by: Jonathan White --- share/translations/keepassxc_en.ts | 24 ++++-- src/browser/BrowserSettings.cpp | 5 ++ src/browser/BrowserSettings.h | 1 + src/browser/BrowserSettingsWidget.cpp | 102 +++++++++++++++++-------- src/browser/BrowserSettingsWidget.h | 6 +- src/browser/BrowserSettingsWidget.ui | 17 +++-- src/browser/NativeMessageInstaller.cpp | 17 +++-- src/browser/NativeMessageInstaller.h | 1 + 8 files changed, 120 insertions(+), 53 deletions(-) diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 29e20cf498..865a42344f 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -1106,14 +1106,6 @@ Do you want to delete the entry? Please see special instructions for browser extension use below - - <b>Error:</b> The custom proxy location cannot be found!<br/>Browser integration WILL NOT WORK without the proxy application. - - - - <b>Warning:</b> The following options can be dangerous! - - Executable Files @@ -1138,6 +1130,22 @@ Do you want to delete the entry? Allow limited access to all entries in connected databases (ignores site access restrictions) + + <b>Warning:</b> Only adjust these settings if necessary. + + + + The custom proxy location does not exist. + + + + <b>Error:</b> The custom proxy location does not exist. Correct this in the advanced settings tab. + + + + <b>Error:</b> The installed proxy executable is missing from the expected location: %1<br/>Please set a custom proxy location in the advanced settings or reinstall the application. + + CloneDialog diff --git a/src/browser/BrowserSettings.cpp b/src/browser/BrowserSettings.cpp index 329659c059..8b1e0b3cb7 100644 --- a/src/browser/BrowserSettings.cpp +++ b/src/browser/BrowserSettings.cpp @@ -200,6 +200,11 @@ QString BrowserSettings::proxyLocation() return m_nativeMessageInstaller.getProxyPath(); } +QString BrowserSettings::proxyLocationAsInstalled() const +{ + return m_nativeMessageInstaller.getInstalledProxyPath(); +} + #ifdef QT_DEBUG QString BrowserSettings::customExtensionId() { diff --git a/src/browser/BrowserSettings.h b/src/browser/BrowserSettings.h index a961a56aea..b61c8d40cf 100644 --- a/src/browser/BrowserSettings.h +++ b/src/browser/BrowserSettings.h @@ -57,6 +57,7 @@ class BrowserSettings QString customProxyLocation(); void setCustomProxyLocation(const QString& location); QString proxyLocation(); + QString proxyLocationAsInstalled() const; #ifdef QT_DEBUG QString customExtensionId(); void setCustomExtensionId(const QString& id); diff --git a/src/browser/BrowserSettingsWidget.cpp b/src/browser/BrowserSettingsWidget.cpp index 6da22b8204..c8d98fa547 100644 --- a/src/browser/BrowserSettingsWidget.cpp +++ b/src/browser/BrowserSettingsWidget.cpp @@ -20,6 +20,7 @@ #include "BrowserSettings.h" #include "config-keepassx.h" +#include "gui/styles/StateColorPalette.h" #include @@ -49,12 +50,9 @@ BrowserSettingsWidget::BrowserSettingsWidget(QWidget* parent) snapInstructions)); // clang-format on - m_ui->warningWidget->setCloseButtonVisible(false); - m_ui->warningWidget->setAutoHideTimeout(-1); - m_ui->warningWidget->setAnimate(false); - m_ui->tabWidget->setEnabled(m_ui->enableBrowserSupport->isChecked()); connect(m_ui->enableBrowserSupport, SIGNAL(toggled(bool)), m_ui->tabWidget, SLOT(setEnabled(bool))); + connect(m_ui->enableBrowserSupport, SIGNAL(toggled(bool)), SLOT(validateProxyLocation())); // Custom Browser option #ifdef Q_OS_WIN @@ -72,10 +70,15 @@ BrowserSettingsWidget::BrowserSettingsWidget(QWidget* parent) connect(m_ui->useCustomProxy, SIGNAL(toggled(bool)), m_ui->customProxyLocation, SLOT(setEnabled(bool))); connect(m_ui->useCustomProxy, SIGNAL(toggled(bool)), m_ui->customProxyLocationBrowseButton, SLOT(setEnabled(bool))); - connect(m_ui->useCustomProxy, SIGNAL(toggled(bool)), SLOT(validateCustomProxyLocation())); - connect(m_ui->customProxyLocation, SIGNAL(editingFinished()), SLOT(validateCustomProxyLocation())); + connect(m_ui->useCustomProxy, SIGNAL(toggled(bool)), SLOT(validateProxyLocation())); + connect(m_ui->customProxyLocation, SIGNAL(editingFinished()), SLOT(validateProxyLocation())); connect(m_ui->customProxyLocationBrowseButton, SIGNAL(clicked()), this, SLOT(showProxyLocationFileDialog())); + m_ui->messageWidget->setVisible(false); + m_ui->messageWidget->setCloseButtonVisible(false); + m_ui->messageWidget->setWordWrap(true); + m_ui->messageWidget->setAutoHideTimeout(MessageWidget::DisableAutoHide); + #ifndef Q_OS_LINUX m_ui->snapWarningLabel->setVisible(false); #endif @@ -90,7 +93,6 @@ BrowserSettingsWidget::BrowserSettingsWidget(QWidget* parent) m_ui->torBrowserSupport->setHidden(true); m_ui->firefoxSupport->setText("Firefox and Tor Browser"); #endif - m_ui->browserGlobalWarningWidget->setVisible(false); #ifndef QT_DEBUG m_ui->customExtensionId->setVisible(false); @@ -154,10 +156,8 @@ void BrowserSettingsWidget::loadSettings() m_ui->customBrowserSupport->setVisible(false); m_ui->customBrowserGroupBox->setVisible(false); // Show notice to user - m_ui->browserGlobalWarningWidget->showMessage(tr("Please see special instructions for browser extension use below"), - MessageWidget::Warning); - m_ui->browserGlobalWarningWidget->setCloseButtonVisible(false); - m_ui->browserGlobalWarningWidget->setAutoHideTimeout(-1); + m_ui->messageWidget->showMessage(tr("Please see special instructions for browser extension use below"), + MessageWidget::Warning); #endif #ifdef KEEPASSXC_DIST_FLATPAK // The sandbox makes custom proxy locations very unintuitive @@ -184,21 +184,50 @@ void BrowserSettingsWidget::loadSettings() #ifdef QT_DEBUG m_ui->customExtensionId->setText(settings->customExtensionId()); #endif - - validateCustomProxyLocation(); + // Validate the complete proxy location dependency - not only in case it is custom, + // to make trouble-shooting for both developer and user easier + validateProxyLocation(); } -void BrowserSettingsWidget::validateCustomProxyLocation() +QString BrowserSettingsWidget::resolveCustomProxyLocation() { - auto path = browserSettings()->customProxyLocation(); + auto settings = browserSettings(); + auto proxyLocation = m_ui->customProxyLocation->text(); + proxyLocation = settings->replaceTildeHomePath(proxyLocation); + return proxyLocation; +} - if (m_ui->useCustomProxy->isChecked() && !QFile::exists(path)) { - m_ui->warningWidget->showMessage(tr("Error: The custom proxy location cannot be found!" - "
Browser integration WILL NOT WORK without the proxy application."), - MessageWidget::Error); - } else { - m_ui->warningWidget->showMessage(tr("Warning: The following options can be dangerous!"), - MessageWidget::Warning); +void BrowserSettingsWidget::validateProxyLocation() +{ + // Reset the UI + m_ui->messageWidget->setVisible(false); + m_ui->customProxyLocation->setStyleSheet(""); + m_ui->customProxyLocation->setToolTip(""); + + if (m_ui->enableBrowserSupport->isChecked()) { + // If we are using a custom proxy location, check if it exists and display warning if not + if (m_ui->useCustomProxy->isChecked()) { + if (!QFile::exists(resolveCustomProxyLocation())) { + StateColorPalette statePalette; + auto color = statePalette.color(StateColorPalette::ColorRole::Error); + m_ui->customProxyLocation->setStyleSheet(QString("QLineEdit { background: %1; }").arg(color.name())); + m_ui->customProxyLocation->setToolTip(tr("The custom proxy location does not exist.")); + + m_ui->messageWidget->showMessage(tr("Error: The custom proxy location does not exist. Correct " + "this in the advanced settings tab."), + MessageWidget::Error); + } + } else { + // Otherwise check if the installed proxy exists + auto expectedProxyLocation = browserSettings()->proxyLocationAsInstalled(); + if (!QFile::exists(expectedProxyLocation)) { + m_ui->messageWidget->showMessage( + tr("Error: The installed proxy executable is missing from the expected location: %1
" + "Please set a custom proxy location in the advanced settings or reinstall the application.") + .arg(expectedProxyLocation), + MessageWidget::Error); + } + } } } @@ -212,7 +241,7 @@ void BrowserSettingsWidget::saveSettings() settings->setMatchUrlScheme(m_ui->matchUrlScheme->isChecked()); settings->setUseCustomProxy(m_ui->useCustomProxy->isChecked()); - settings->setCustomProxyLocation(browserSettings()->replaceTildeHomePath(m_ui->customProxyLocation->text())); + settings->setCustomProxyLocation(resolveCustomProxyLocation()); settings->setUpdateBinaryPath(m_ui->updateBinaryPath->isChecked()); settings->setAllowGetDatabaseEntriesRequest(m_ui->allowGetDatabaseEntriesRequest->isChecked()); @@ -254,14 +283,25 @@ void BrowserSettingsWidget::showProxyLocationFileDialog() #else QString fileTypeFilter(QString("%1 (*)").arg(tr("Executable Files"))); #endif - auto proxyLocation = QFileDialog::getOpenFileName(this, - tr("Select custom proxy location"), - QFileInfo(QCoreApplication::applicationDirPath()).filePath(), - fileTypeFilter); - - proxyLocation = browserSettings()->replaceHomePath(proxyLocation); - m_ui->customProxyLocation->setText(proxyLocation); - validateCustomProxyLocation(); + + auto initialFilePath = resolveCustomProxyLocation(); + if (QFileInfo::exists(initialFilePath)) { + initialFilePath = QFileInfo(initialFilePath).filePath(); + } else { + // ignore current status and set as it would be installed + initialFilePath = QFileInfo(browserSettings()->proxyLocationAsInstalled()).filePath(); + } + + QString proxyLocation = + QFileDialog::getOpenFileName(this, tr("Select custom proxy location"), initialFilePath, fileTypeFilter); + + if (!proxyLocation.isEmpty()) { + proxyLocation = browserSettings()->replaceHomePath(proxyLocation); + m_ui->customProxyLocation->setText(proxyLocation); + validateProxyLocation(); + } else { + // do not overwrite old proxy setting + } } void BrowserSettingsWidget::showCustomBrowserLocationFileDialog() diff --git a/src/browser/BrowserSettingsWidget.h b/src/browser/BrowserSettingsWidget.h index d84051d0ec..d6dec8ab13 100644 --- a/src/browser/BrowserSettingsWidget.h +++ b/src/browser/BrowserSettingsWidget.h @@ -32,7 +32,7 @@ class BrowserSettingsWidget : public QWidget public: explicit BrowserSettingsWidget(QWidget* parent = nullptr); - ~BrowserSettingsWidget(); + ~BrowserSettingsWidget() override; public slots: void loadSettings(); @@ -40,10 +40,12 @@ public slots: private slots: void showProxyLocationFileDialog(); - void validateCustomProxyLocation(); + void validateProxyLocation(); void showCustomBrowserLocationFileDialog(); private: + QString resolveCustomProxyLocation(); + QScopedPointer m_ui; }; diff --git a/src/browser/BrowserSettingsWidget.ui b/src/browser/BrowserSettingsWidget.ui index 13af62a40a..2c9d085c23 100644 --- a/src/browser/BrowserSettingsWidget.ui +++ b/src/browser/BrowserSettingsWidget.ui @@ -27,7 +27,7 @@ 0 - + @@ -267,12 +267,15 @@ - - - - 0 - 0 - + + + <b>Warning:</b> Only adjust these settings if necessary. + + + false + + + 2 diff --git a/src/browser/NativeMessageInstaller.cpp b/src/browser/NativeMessageInstaller.cpp index 2d5054ec92..7409989ab7 100644 --- a/src/browser/NativeMessageInstaller.cpp +++ b/src/browser/NativeMessageInstaller.cpp @@ -272,17 +272,24 @@ QString constructFlatpakPath() #endif /** - * Gets the path to keepassxc-proxy binary - * - * @param location Custom proxy path - * @return path Path to keepassxc-proxy + * Returns the effective proxy path used to build the native messaging JSON script */ QString NativeMessageInstaller::getProxyPath() const { + QString result; if (browserSettings()->useCustomProxy()) { - return browserSettings()->customProxyLocation(); + result = browserSettings()->customProxyLocation(); + } else { + result = getInstalledProxyPath(); } + return result; +} +/** + * Returns the original proxy path at the time of installation + */ +QString NativeMessageInstaller::getInstalledProxyPath() const +{ QString path; #if defined(KEEPASSXC_DIST_APPIMAGE) path = QProcessEnvironment::systemEnvironment().value("APPIMAGE"); diff --git a/src/browser/NativeMessageInstaller.h b/src/browser/NativeMessageInstaller.h index 256dd0c812..f54d688850 100644 --- a/src/browser/NativeMessageInstaller.h +++ b/src/browser/NativeMessageInstaller.h @@ -33,6 +33,7 @@ class NativeMessageInstaller bool isBrowserEnabled(BrowserShared::SupportedBrowsers browser); QString getProxyPath() const; + QString getInstalledProxyPath() const; void updateBinaryPaths(); private: