Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Browser integration settings rework and KeePassHTTP deprecation #1392

Merged
merged 4 commits into from
Jan 19, 2018
Merged

Browser integration settings rework and KeePassHTTP deprecation #1392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2b3181
Rework browser integration settings
phoerious Jan 16, 2018
f520a0f
Deprecate KeePassHTTP and introduce WITH_XC_NETWORKING CMake option
phoerious Jan 16, 2018
30f77b0
Improve KMessageWidget visuals
phoerious Jan 19, 2018
a6fd52d
Show KeePassHTTP deprecation notice three times and use MessageWidget
phoerious Jan 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Copyright (C) 2010 Felix Geyer <[email protected]>
# Copyright (C) 2017 KeePassXC Team <[email protected]>
# Copyright (C) 2010 Felix Geyer <[email protected]>
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
Expand Down Expand Up @@ -41,10 +41,17 @@ option(WITH_COVERAGE "Use to build with coverage tests (GCC only)." OFF)
option(WITH_APP_BUNDLE "Enable Application Bundle for macOS" ON)

option(WITH_XC_AUTOTYPE "Include Auto-Type." ON)
option(WITH_XC_HTTP "Include KeePassHTTP and Custom Icon Downloads." OFF)
option(WITH_XC_NETWORKING "Include networking code (e.g. for downlading website icons)." OFF)
option(WITH_XC_BROWSER "Include browser integration with keepassxc-browser." OFF)
option(WITH_XC_HTTP "Include KeePassHTTP-compatible browser integration (deprecated, implies WITH_NETWORKING)." OFF)
option(WITH_XC_YUBIKEY "Include YubiKey support." OFF)
option(WITH_XC_SSHAGENT "Include SSH agent support." OFF)
option(WITH_XC_BROWSER "Include browser extension support." OFF)

if(WITH_XC_HTTP)
message(WARNING "KeePassHTTP support has been deprecated and will be removed in a future version. Please use WITH_XC_BROWSER instead!\n"
"For enabling / disabling network access code, WITH_XC_HTTP has been replaced by WITH_XC_NETWORKING.")
set(WITH_XC_NETWORKING ON CACHE BOOL "Include networking code (e.g. for downlading website icons)." FORCE)
endif()

# Process ui files automatically from source files
set(CMAKE_AUTOUIC ON)
Expand Down
21 changes: 14 additions & 7 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,15 +187,21 @@ set(keepassx_SOURCES_MAINEXE
main.cpp
)

add_feature_info(AutoType WITH_XC_AUTOTYPE "Automatic password typing")
add_feature_info(KeePassHTTP WITH_XC_HTTP "Browser integration compatible with ChromeIPass and PassIFox")
add_feature_info(YubiKey WITH_XC_YUBIKEY "YubiKey HMAC-SHA1 challenge-response")
add_feature_info(Auto-Type WITH_XC_AUTOTYPE "Automatic password typing")
add_feature_info(Networking WITH_XC_NETWORKING "Compile KeePassXC with network access code (e.g. for downloading website icons)")
add_feature_info(keepassxc-browser WITH_XC_BROWSER "Browser integration with keepassxc-browser")
add_feature_info(KeePassHTTP WITH_XC_HTTP "Browser integration compatible with ChromeIPass and PassIFox (deprecated, implies Networking)")
add_feature_info(SSHAgent WITH_XC_SSHAGENT "SSH agent integration compatible with KeeAgent")
add_feature_info(keepassxc-browser WITH_XC_BROWSER "KeePassXC browser support with keepassxc-browser")
add_feature_info(YubiKey WITH_XC_YUBIKEY "YubiKey HMAC-SHA1 challenge-response")


add_subdirectory(http)
if(WITH_XC_HTTP)
set(keepasshttp_LIB keepasshttp qhttp Qt5::Network)
add_subdirectory(http)
set(keepasshttp_LIB keepasshttp)
endif()
if(WITH_XC_NETWORKING)
add_subdirectory(http/qhttp)
set(keepassxcnetwork_LIB qhttp Qt5::Network)
endif()

set(BROWSER_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/browser)
Expand Down Expand Up @@ -247,8 +253,9 @@ add_library(keepassx_core STATIC ${keepassx_SOURCES})

set_target_properties(keepassx_core PROPERTIES COMPILE_DEFINITIONS KEEPASSX_BUILDING_CORE)
target_link_libraries(keepassx_core
${keepasshttp_LIB}
${keepassxcbrowser_LIB}
${keepasshttp_LIB}
${keepassxcnetwork_LIB}
${autotype_LIB}
${sshagent_LIB}
${YUBIKEY_LIBRARIES}
Expand Down
26 changes: 24 additions & 2 deletions src/browser/BrowserOptionDialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "core/FilePath.h"

#include <QMessageBox>
#include <QFileDialog>

BrowserOptionDialog::BrowserOptionDialog(QWidget* parent) :
QWidget(parent),
Expand All @@ -32,15 +33,18 @@ BrowserOptionDialog::BrowserOptionDialog(QWidget* parent) :
connect(m_ui->removeSharedEncryptionKeys, SIGNAL(clicked()), this, SIGNAL(removeSharedEncryptionKeys()));
connect(m_ui->removeStoredPermissions, SIGNAL(clicked()), this, SIGNAL(removeStoredPermissions()));

m_ui->warningWidget->showMessage(tr("The following options can be dangerous!\nChange them only if you know what you are doing."), MessageWidget::Warning);
m_ui->warningWidget->setIcon(FilePath::instance()->icon("status", "dialog-warning"));
m_ui->warningWidget->showMessage(tr("<b>Warning:</b> The following options can be dangerous!"), MessageWidget::Warning);
m_ui->warningWidget->setCloseButtonVisible(false);
m_ui->warningWidget->setAutoHideTimeout(-1);

m_ui->tabWidget->setEnabled(m_ui->enableBrowserSupport->isChecked());
connect(m_ui->enableBrowserSupport, SIGNAL(toggled(bool)), m_ui->tabWidget, SLOT(setEnabled(bool)));

m_ui->customProxyLocation->setEnabled(m_ui->useCustomProxy->isChecked());
m_ui->customProxyLocationBrowseButton->setEnabled(m_ui->useCustomProxy->isChecked());
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->customProxyLocationBrowseButton, SIGNAL(clicked()), this, SLOT(showProxyLocationFileDialog()));
}

BrowserOptionDialog::~BrowserOptionDialog()
Expand All @@ -57,6 +61,11 @@ void BrowserOptionDialog::loadSettings()
m_ui->unlockDatabase->setChecked(settings.unlockDatabase());
m_ui->matchUrlScheme->setChecked(settings.matchUrlScheme());

// hide unimplemented options
// TODO: fix this
m_ui->showNotification->hide();
m_ui->bestMatchOnly->hide();

if (settings.sortByUsername()) {
m_ui->sortByUsername->setChecked(true);
} else {
Expand Down Expand Up @@ -102,3 +111,16 @@ void BrowserOptionDialog::saveSettings()
settings.setFirefoxSupport(m_ui->firefoxSupport->isChecked());
settings.setVivaldiSupport(m_ui->vivaldiSupport->isChecked());
}

void BrowserOptionDialog::showProxyLocationFileDialog()
{
#ifdef Q_OS_WIN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using #if defined(Q_OS_WIN) elsewhere, we should stick with one notation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we have both. defined() is necessary of you combine different conditions. I can change it, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, fine by me.
But at some point we should convert every ifdef to if defined IMHO

QString fileTypeFilter(tr("Executable Files (*.exe);;All Files (*.*)"));
#else
QString fileTypeFilter(tr("Executable Files (*.*)"));
#endif
auto proxyLocation = QFileDialog::getOpenFileName(this, tr("Select custom proxy location"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use FileDialog::getOpenFileName()

Copy link
Member Author

@phoerious phoerious Jan 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must admit I forgot about that class. But I don't think I should use it here. The main purpose of FileDialog is to remember the last open/save location. We don't want that here. In fact, it would be very annoying if the next time I click "Open Database" the selected directory is the KeePassXC install folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use FileDialog::setNextForgetDialog() before calling getOpenFileName() to not save the final destination, and you can specify the dir argument so you won't be placed in the LastDir.

Using our FileDialog implementation helps with future tests (since implements nextFileName), fixes focus on macOS and if in the future we will modify something that will get propagated here as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set a few additional flags for the QFileDialog, which can't be set via the static method, so I had to change this to a custom instantiation of QFileDialog. That makes it impossible to use FileDialog here. I therefore added the extra activateWindow() fix and would suggest to postpone this to a refactor of FileDialog, during which we make it a subclass of QFileDialog.

QFileInfo(QCoreApplication::applicationDirPath()).filePath(),
fileTypeFilter);
m_ui->customProxyLocation->setText(proxyLocation);
}
3 changes: 3 additions & 0 deletions src/browser/BrowserOptionDialog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ public slots:
void removeSharedEncryptionKeys();
void removeStoredPermissions();

private slots:
void showProxyLocationFileDialog();

private:
QScopedPointer<Ui::BrowserOptionDialog> m_ui;
};
Expand Down
Loading