From 884386c924192902dc9500a58f9cbdfe22a0a4fd Mon Sep 17 00:00:00 2001 From: Michael Duersch <8960738+ekimd@users.noreply.github.com> Date: Sun, 17 Sep 2023 16:21:05 -0600 Subject: [PATCH] Allow groups to restrict by browser integration key (#6437) --- share/translations/keepassxc_en.ts | 14 +++++- src/browser/BrowserService.cpp | 30 +++++++++--- src/browser/BrowserService.h | 4 ++ src/core/Group.cpp | 15 ++++++ src/core/Group.h | 1 + src/gui/group/EditGroupWidget.cpp | 60 +++++++++++++++++++++++ src/gui/group/EditGroupWidget.h | 4 ++ src/gui/group/EditGroupWidgetBrowser.ui | 18 +++++++ tests/TestBrowser.cpp | 65 +++++++++++++++++++++++++ tests/TestBrowser.h | 1 + 10 files changed, 205 insertions(+), 7 deletions(-) diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 648c54347..34c07934e 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -953,7 +953,7 @@ Do you want to overwrite the Passkey in %1 - %2? - KeePassXC - New key association request + Disable @@ -972,6 +972,10 @@ Do you want to overwrite the Passkey in %1 - %2? KeePassXC - Delete entry + + KeePassXC - New key association request + + BrowserSettingsWidget @@ -3209,6 +3213,14 @@ Would you like to correct it? Omit WWW subdomain from matching toggle for this and sub groups + + Restrict matching to given browser key: + + + + Restrict matching to given browser key toggle for this and sub groups + + EditGroupWidgetKeeShare diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index 6903eceef..bdae48866 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -64,6 +64,7 @@ const QString BrowserService::OPTION_HIDE_ENTRY = QStringLiteral("BrowserHideEnt const QString BrowserService::OPTION_ONLY_HTTP_AUTH = QStringLiteral("BrowserOnlyHttpAuth"); const QString BrowserService::OPTION_NOT_HTTP_AUTH = QStringLiteral("BrowserNotHttpAuth"); const QString BrowserService::OPTION_OMIT_WWW = QStringLiteral("BrowserOmitWww"); +const QString BrowserService::OPTION_RESTRICT_KEY = QStringLiteral("BrowserRestrictKey"); Q_GLOBAL_STATIC(BrowserService, s_browserService); @@ -947,6 +948,7 @@ bool BrowserService::deleteEntry(const QString& uuid) QList BrowserService::searchEntries(const QSharedPointer& db, const QString& siteUrl, const QString& formUrl, + const QStringList& keys, bool passkey) { QList entries; @@ -961,6 +963,12 @@ QList BrowserService::searchEntries(const QSharedPointer& db, continue; } + // If a key restriction is specified and not contained in the keys list then skip this group. + auto restrictKey = group->resolveCustomDataString(BrowserService::OPTION_RESTRICT_KEY); + if (!restrictKey.isEmpty() && !keys.contains(restrictKey)) { + continue; + } + const auto omitWwwSubdomain = group->resolveCustomDataTriState(BrowserService::OPTION_OMIT_WWW) == Group::Enable; @@ -997,30 +1005,35 @@ QList BrowserService::searchEntries(const QString& siteUrl, const StringPairList& keyList, bool passkey) { - // Check if database is connected with KeePassXC-Browser + // Check if database is connected with KeePassXC-Browser. If so, return browser key (otherwise empty) auto databaseConnected = [&](const QSharedPointer& db) { for (const StringPair& keyPair : keyList) { QString key = db->metadata()->customData()->value(CustomData::BrowserKeyPrefix + keyPair.first); if (!key.isEmpty() && keyPair.second == key) { - return true; + return keyPair.first; } } - return false; + return QString(); }; // Get the list of databases to search QList> databases; + QStringList keys; if (browserSettings()->searchInAllDatabases()) { for (auto dbWidget : getMainWindow()->getOpenDatabases()) { auto db = dbWidget->database(); - if (db && databaseConnected(dbWidget->database())) { + auto key = databaseConnected(dbWidget->database()); + if (db && !key.isEmpty()) { databases << db; + keys << key; } } } else { const auto& db = getDatabase(); - if (databaseConnected(db)) { + auto key = databaseConnected(db); + if (!key.isEmpty()) { databases << db; + keys << key; } } @@ -1029,13 +1042,18 @@ QList BrowserService::searchEntries(const QString& siteUrl, QList entries; do { for (const auto& db : databases) { - entries << searchEntries(db, siteUrl, formUrl, passkey); + entries << searchEntries(db, siteUrl, formUrl, keys, passkey); } } while (entries.isEmpty() && removeFirstDomain(hostname)); return entries; } +QString BrowserService::decodeCustomDataRestrictKey(const QString& key) +{ + return key.isEmpty() ? tr("Disable") : key; +} + void BrowserService::requestGlobalAutoType(const QString& search) { emit osUtils->globalShortcutTriggered("autotype", search); diff --git a/src/browser/BrowserService.h b/src/browser/BrowserService.h index 7550ad494..c8bbafa0f 100644 --- a/src/browser/BrowserService.h +++ b/src/browser/BrowserService.h @@ -119,6 +119,8 @@ public: QJsonArray findEntries(const EntryParameters& entryParameters, const StringPairList& keyList, bool* entriesFound); void requestGlobalAutoType(const QString& search); + static QString decodeCustomDataRestrictKey(const QString& key); + static const QString KEEPASSXCBROWSER_NAME; static const QString KEEPASSXCBROWSER_OLD_NAME; static const QString OPTION_SKIP_AUTO_SUBMIT; @@ -126,6 +128,7 @@ public: static const QString OPTION_ONLY_HTTP_AUTH; static const QString OPTION_NOT_HTTP_AUTH; static const QString OPTION_OMIT_WWW; + static const QString OPTION_RESTRICT_KEY; signals: void requestUnlock(); @@ -157,6 +160,7 @@ private: QList searchEntries(const QSharedPointer& db, const QString& siteUrl, const QString& formUrl, + const QStringList& keys = {}, bool passkey = false); QList searchEntries(const QString& siteUrl, const QString& formUrl, const StringPairList& keyList, bool passkey = false); diff --git a/src/core/Group.cpp b/src/core/Group.cpp index 649833178..43fda39a4 100644 --- a/src/core/Group.cpp +++ b/src/core/Group.cpp @@ -288,6 +288,21 @@ void Group::setCustomDataTriState(const QString& key, const Group::TriState& val } } +// Note that this returns an empty string both if the key is missing *or* if the key is present but value is empty. +QString Group::resolveCustomDataString(const QString& key, bool checkParent) const +{ + // If not defined, check our parent up to the root group + if (!m_customData->contains(key)) { + if (!m_parent || !checkParent) { + return QString(); + } else { + return m_parent->resolveCustomDataString(key); + } + } + + return m_customData->value(key); +} + bool Group::equals(const Group* other, CompareItemOptions options) const { if (!other) { diff --git a/src/core/Group.h b/src/core/Group.h index 67f14d776..e934e5a65 100644 --- a/src/core/Group.h +++ b/src/core/Group.h @@ -102,6 +102,7 @@ public: const CustomData* customData() const; Group::TriState resolveCustomDataTriState(const QString& key, bool checkParent = true) const; void setCustomDataTriState(const QString& key, const Group::TriState& value); + QString resolveCustomDataString(const QString& key, bool checkParent = true) const; const Group* previousParentGroup() const; QUuid previousParentGroupUuid() const; diff --git a/src/gui/group/EditGroupWidget.cpp b/src/gui/group/EditGroupWidget.cpp index 1b473c1fc..bf38caf2e 100644 --- a/src/gui/group/EditGroupWidget.cpp +++ b/src/gui/group/EditGroupWidget.cpp @@ -196,6 +196,7 @@ void EditGroupWidget::loadGroup(Group* group, bool create, const QSharedPointer< auto inheritOnlyHttp = false; auto inheritNoHttp = false; auto inheritOmitWww = false; + auto inheritRestrictKey = QString(); auto parent = group->parentGroup(); if (parent) { @@ -204,6 +205,7 @@ void EditGroupWidget::loadGroup(Group* group, bool create, const QSharedPointer< inheritOnlyHttp = parent->resolveCustomDataTriState(BrowserService::OPTION_ONLY_HTTP_AUTH); inheritNoHttp = parent->resolveCustomDataTriState(BrowserService::OPTION_NOT_HTTP_AUTH); inheritOmitWww = parent->resolveCustomDataTriState(BrowserService::OPTION_OMIT_WWW); + inheritRestrictKey = parent->resolveCustomDataString(BrowserService::OPTION_RESTRICT_KEY); } // If the page has not been created at all, some of the elements are null @@ -219,6 +221,7 @@ void EditGroupWidget::loadGroup(Group* group, bool create, const QSharedPointer< addTriStateItems(m_browserUi->browserIntegrationOnlyHttpAuthComboBox, inheritOnlyHttp); addTriStateItems(m_browserUi->browserIntegrationNotHttpAuthComboBox, inheritNoHttp); addTriStateItems(m_browserUi->browserIntegrationOmitWwwCombobox, inheritOmitWww); + addRestrictKeyComboBoxItems(m_db->metadata()->customData()->keys(), inheritRestrictKey); m_browserUi->browserIntegrationHideEntriesComboBox->setCurrentIndex( indexFromTriState(group->resolveCustomDataTriState(BrowserService::OPTION_HIDE_ENTRY, false))); @@ -230,6 +233,7 @@ void EditGroupWidget::loadGroup(Group* group, bool create, const QSharedPointer< indexFromTriState(group->resolveCustomDataTriState(BrowserService::OPTION_NOT_HTTP_AUTH, false))); m_browserUi->browserIntegrationOmitWwwCombobox->setCurrentIndex( indexFromTriState(group->resolveCustomDataTriState(BrowserService::OPTION_OMIT_WWW, false))); + setRestrictKeyComboBoxIndex(group); } else if (hasPage(m_browserWidget)) { setPageHidden(m_browserWidget, true); } @@ -303,6 +307,7 @@ void EditGroupWidget::apply() m_temporaryGroup->setCustomDataTriState( BrowserService::OPTION_OMIT_WWW, triStateFromIndex(m_browserUi->browserIntegrationOmitWwwCombobox->currentIndex())); + setRestrictKeyCustomData(m_temporaryGroup->customData()); } #endif @@ -444,3 +449,58 @@ Group::TriState EditGroupWidget::triStateFromIndex(int index) return Group::Inherit; } } + +#ifdef WITH_XC_BROWSER +void EditGroupWidget::addRestrictKeyComboBoxItems(QStringList const& keyList, QString inheritValue) +{ + auto comboBox = m_browserUi->browserIntegrationRestrictKeyCombobox; + + comboBox->clear(); + comboBox->addItem( + tr("Inherit from parent group (%1)").arg(BrowserService::decodeCustomDataRestrictKey(inheritValue))); + comboBox->addItem(tr("Disable")); + + comboBox->insertSeparator(2); + + // Add all the browser keys to the combobox + for (const QString& key : keyList) { + if (key.startsWith(CustomData::BrowserKeyPrefix)) { + auto strippedKey = key; + strippedKey.remove(CustomData::BrowserKeyPrefix); + comboBox->addItem(strippedKey); + } + } +} + +void EditGroupWidget::setRestrictKeyComboBoxIndex(const Group* group) +{ + auto comboBox = m_browserUi->browserIntegrationRestrictKeyCombobox; + + if (!group || !group->customData()->contains(BrowserService::OPTION_RESTRICT_KEY)) { + comboBox->setCurrentIndex(0); + return; + } + + auto key = group->customData()->value(BrowserService::OPTION_RESTRICT_KEY); + if (key.isEmpty()) { + comboBox->setCurrentIndex(1); + } else { + comboBox->setCurrentText(key); + } +} + +// Set the customData regarding OPTION_RESTRICT_KEY +void EditGroupWidget::setRestrictKeyCustomData(CustomData* customData) +{ + auto comboBox = m_browserUi->browserIntegrationRestrictKeyCombobox; + auto key = BrowserService::OPTION_RESTRICT_KEY; + auto idx = comboBox->currentIndex(); + if (idx == 0) { + customData->remove(key); + } else if (idx == 1) { + customData->set(key, QString()); + } else { + customData->set(key, comboBox->currentText()); + } +} +#endif diff --git a/src/gui/group/EditGroupWidget.h b/src/gui/group/EditGroupWidget.h index 8e8d2c85b..35a84d768 100644 --- a/src/gui/group/EditGroupWidget.h +++ b/src/gui/group/EditGroupWidget.h @@ -81,6 +81,10 @@ private: Group::TriState triStateFromIndex(int index); void setupModifiedTracking(); + void addRestrictKeyComboBoxItems(QStringList const& keyList, QString inheritValue); + void setRestrictKeyComboBoxIndex(const Group* group); + void setRestrictKeyCustomData(CustomData* customData); + const QScopedPointer m_mainUi; QPointer m_editGroupWidgetMain; diff --git a/src/gui/group/EditGroupWidgetBrowser.ui b/src/gui/group/EditGroupWidgetBrowser.ui index dfe4b5971..8d94693fd 100644 --- a/src/gui/group/EditGroupWidgetBrowser.ui +++ b/src/gui/group/EditGroupWidgetBrowser.ui @@ -136,6 +136,23 @@ + + + Restrict matching to given browser key: + + + Qt::AlignLeading|Qt::AlignLeft|Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter + + + + + + + Restrict matching to given browser key toggle for this and sub groups + + + + Qt::Vertical @@ -158,6 +175,7 @@ browserIntegrationOnlyHttpAuthComboBox browserIntegrationNotHttpAuthComboBox browserIntegrationOmitWwwCombobox + browserIntegrationRestrictKeyCombobox diff --git a/tests/TestBrowser.cpp b/tests/TestBrowser.cpp index 19dbba8f4..09bd94cfe 100644 --- a/tests/TestBrowser.cpp +++ b/tests/TestBrowser.cpp @@ -675,3 +675,68 @@ void TestBrowser::testBestMatchingWithAdditionalURLs() QCOMPARE(sorted.length(), 1); QCOMPARE(sorted[0]->url(), urls[0]); } + +void TestBrowser::testRestrictBrowserKey() +{ + auto db = QSharedPointer::create(); + auto* root = db->rootGroup(); + + // Group 0 (root): No browser key restriction given + QStringList urlsRoot = {"https://example.com/0"}; + auto entriesRoot = createEntries(urlsRoot, root); + + // Group 1: restricted to browser with 'key1' + auto* group1 = new Group(); + group1->setParent(root); + group1->setName("TestGroup1"); + group1->customData()->set(BrowserService::OPTION_RESTRICT_KEY, "key1"); + QStringList urls1 = {"https://example.com/1"}; + auto entries1 = createEntries(urls1, group1); + + // Group 2: restricted to browser with 'key2' + auto* group2 = new Group(); + group2->setParent(root); + group2->setName("TestGroup2"); + group2->customData()->set(BrowserService::OPTION_RESTRICT_KEY, "key2"); + QStringList urls2 = {"https://example.com/2"}; + auto entries2 = createEntries(urls2, group2); + + // Group 2b: inherits parent group (2) restriction + auto* group2b = new Group(); + group2b->setParent(group2); + group2b->setName("TestGroup2b"); + QStringList urls2b = {"https://example.com/2b"}; + auto entries2b = createEntries(urls2b, group2b); + + // Group 3: inherits parent group (root) - any browser can see + auto* group3 = new Group(); + group3->setParent(root); + group3->setName("TestGroup3"); + QStringList urls3 = {"https://example.com/3"}; + auto entries3 = createEntries(urls3, group3); + + // Browser 'key0': Groups 1 and 2 are excluded, so entries 0 and 3 will be found + auto siteUrl = QString("https://example.com"); + auto result = m_browserService->searchEntries(db, siteUrl, siteUrl, {"key0"}); + auto sorted = m_browserService->sortEntries(result, siteUrl, siteUrl); + QCOMPARE(sorted.size(), 2); + QCOMPARE(sorted[0]->url(), QString("https://example.com/3")); + QCOMPARE(sorted[1]->url(), QString("https://example.com/0")); + + // Browser 'key1': Group 2 will be excluded, so entries 0, 1, and 3 will be found + result = m_browserService->searchEntries(db, siteUrl, siteUrl, {"key1"}); + sorted = m_browserService->sortEntries(result, siteUrl, siteUrl); + QCOMPARE(sorted.size(), 3); + QCOMPARE(sorted[0]->url(), QString("https://example.com/3")); + QCOMPARE(sorted[1]->url(), QString("https://example.com/1")); + QCOMPARE(sorted[2]->url(), QString("https://example.com/0")); + + // Browser 'key2': Group 1 will be excluded, so entries 0, 2, 2b, 3 will be found + result = m_browserService->searchEntries(db, siteUrl, siteUrl, {"key2"}); + sorted = m_browserService->sortEntries(result, siteUrl, siteUrl); + QCOMPARE(sorted.size(), 4); + QCOMPARE(sorted[0]->url(), QString("https://example.com/3")); + QCOMPARE(sorted[1]->url(), QString("https://example.com/2b")); + QCOMPARE(sorted[2]->url(), QString("https://example.com/2")); + QCOMPARE(sorted[3]->url(), QString("https://example.com/0")); +} diff --git a/tests/TestBrowser.h b/tests/TestBrowser.h index 48ac3b1cd..6b53a577d 100644 --- a/tests/TestBrowser.h +++ b/tests/TestBrowser.h @@ -49,6 +49,7 @@ private slots: void testSubdomainsAndPaths(); void testBestMatchingCredentials(); void testBestMatchingWithAdditionalURLs(); + void testRestrictBrowserKey(); private: QList createEntries(QStringList& urls, Group* root) const;