Refactor browser Access Control Dialog (#9607)

share/translations/keepassxc_en.ts

@@ -816,6 +816,10 @@ Ctrl+4 - Use Virtual Keyboard (Windows Only)&lt;/p&gt;</source>
         <source>Disable for this site</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Undo</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>BrowserEntrySaveDialog</name>

src/browser/BrowserAccessControlDialog.cpp

@@ -1,6 +1,6 @@
 /*
+ *  Copyright (C) 2023 KeePassXC Team <team@keepassxc.org>
  *  Copyright (C) 2013 Francois Ferrand
- *  Copyright (C) 2022 KeePassXC Team <team@keepassxc.org>
  *
  *  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
@@ -21,12 +21,11 @@
 #include <QUrl>
 
 #include "core/Entry.h"
-#include <QCloseEvent>
+#include "gui/Icons.h"
 
 BrowserAccessControlDialog::BrowserAccessControlDialog(QWidget* parent)
     : QDialog(parent)
     , m_ui(new Ui::BrowserAccessControlDialog())
-    , m_entriesAccepted(false)
 {
     setWindowFlags(windowFlags() | Qt::WindowStaysOnTopHint);
 
@@ -34,13 +33,22 @@ BrowserAccessControlDialog::BrowserAccessControlDialog(QWidget* parent)
 
     connect(m_ui->allowButton, SIGNAL(clicked()), SLOT(accept()));
     connect(m_ui->denyButton, SIGNAL(clicked()), SLOT(reject()));
+    connect(m_ui->itemsTable, SIGNAL(cellDoubleClicked(int, int)), this, SLOT(accept()));
+    connect(m_ui->itemsTable->selectionModel(),
+            SIGNAL(selectionChanged(QItemSelection, QItemSelection)),
+            this,
+            SLOT(selectionChanged()));
+    connect(m_ui->itemsTable, SIGNAL(acceptSelections()), SLOT(accept()));
+    connect(m_ui->itemsTable, SIGNAL(focusInWithoutSelections()), this, SLOT(selectionChanged()));
 }
 
 BrowserAccessControlDialog::~BrowserAccessControlDialog()
 {
 }
 
-void BrowserAccessControlDialog::setItems(const QList<Entry*>& items, const QString& urlString, bool httpAuth)
+void BrowserAccessControlDialog::setEntries(const QList<Entry*>& entriesToConfirm,
+                                            const QString& urlString,
+                                            bool httpAuth)
 {
     QUrl url(urlString);
     m_ui->siteLabel->setText(m_ui->siteLabel->text().arg(
@@ -49,60 +57,114 @@ void BrowserAccessControlDialog::setItems(const QList<Entry*>& items, const QStr
     m_ui->rememberDecisionCheckBox->setVisible(!httpAuth);
     m_ui->rememberDecisionCheckBox->setChecked(false);
 
-    m_ui->itemsTable->setRowCount(items.count());
+    m_ui->itemsTable->setRowCount(entriesToConfirm.count());
     m_ui->itemsTable->setColumnCount(2);
 
     int row = 0;
-    for (const auto& entry : items) {
+    for (const auto& entry : entriesToConfirm) {
-        auto item = new QTableWidgetItem();
+        addEntryToList(entry, row);
-        item->setText(entry->title() + " - " + entry->username());
-        item->setData(Qt::UserRole, row);
-        item->setCheckState(Qt::Checked);
-        item->setFlags(item->flags() | Qt::ItemIsUserCheckable);
-        m_ui->itemsTable->setItem(row, 0, item);
-
-        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) {
-                reject();
-            }
-        });
-        m_ui->itemsTable->setCellWidget(row, 1, disableButton);
         ++row;
     }
     m_ui->itemsTable->resizeColumnsToContents();
     m_ui->itemsTable->horizontalHeader()->setSectionResizeMode(0, QHeaderView::Stretch);
+    m_ui->itemsTable->selectAll();
     m_ui->allowButton->setFocus();
 }
 
+void BrowserAccessControlDialog::addEntryToList(Entry* entry, int row)
+{
+    auto item = new QTableWidgetItem();
+    item->setText(entry->title() + " - " + entry->username());
+    item->setData(Qt::UserRole, row);
+    item->setFlags(item->flags() | Qt::ItemIsSelectable);
+    m_ui->itemsTable->setItem(row, 0, item);
+
+    auto disableButton = new QPushButton();
+    disableButton->setIcon(icons()->icon("entry-delete"));
+    disableButton->setToolTip(tr("Disable for this site"));
+
+    connect(disableButton, &QAbstractButton::pressed, [&, item, disableButton] {
+        auto font = item->font();
+        if (item->flags() == Qt::NoItemFlags) {
+            item->setFlags(Qt::ItemIsEnabled | Qt::ItemIsSelectable);
+            item->setSelected(true);
+
+            font.setStrikeOut(false);
+            item->setFont(font);
+
+            disableButton->setIcon(icons()->icon("entry-delete"));
+            disableButton->setToolTip(tr("Disable for this site"));
+            m_ui->rememberDecisionCheckBox->setEnabled(true);
+        } else {
+            item->setFlags(Qt::NoItemFlags);
+            item->setSelected(false);
+
+            font.setStrikeOut(true);
+            item->setFont(font);
+
+            disableButton->setIcon(icons()->icon("entry-restore"));
+            disableButton->setToolTip(tr("Undo"));
+
+            // Disable Remember checkbox if all items are disabled
+            auto areAllDisabled = BrowserAccessControlDialog::areAllDisabled();
+            m_ui->rememberDecisionCheckBox->setEnabled(!areAllDisabled);
+        }
+    });
+
+    m_ui->itemsTable->setCellWidget(row, 1, disableButton);
+}
+
 bool BrowserAccessControlDialog::remember() const
 {
     return m_ui->rememberDecisionCheckBox->isChecked();
 }
 
-QList<QTableWidgetItem*> BrowserAccessControlDialog::getSelectedEntries() const
+QList<QTableWidgetItem*> BrowserAccessControlDialog::getEntries(SelectionType selectionType) const
 {
     QList<QTableWidgetItem*> selected;
-    for (int i = 0; i < m_ui->itemsTable->rowCount(); ++i) {
+    for (auto& item : getAllItems()) {
-        auto item = m_ui->itemsTable->item(i, 0);
+        // Add to list depending on selection type and item status
-        if (item->checkState() == Qt::Checked) {
+        if ((selectionType == SelectionType::Selected && item->isSelected())
+            || (selectionType == SelectionType::NonSelected && !item->isSelected())
+            || (selectionType == SelectionType::Disabled && item->flags() == Qt::NoItemFlags)) {
             selected.append(item);
         }
     }
     return selected;
 }
 
-QList<QTableWidgetItem*> BrowserAccessControlDialog::getNonSelectedEntries() const
+void BrowserAccessControlDialog::selectionChanged()
 {
-    QList<QTableWidgetItem*> notSelected;
+    auto selectedRows = m_ui->itemsTable->selectionModel()->selectedRows();
-    for (int i = 0; i < m_ui->itemsTable->rowCount(); ++i) {
+
-        auto item = m_ui->itemsTable->item(i, 0);
+    m_ui->allowButton->setEnabled(!selectedRows.isEmpty());
-        if (item->checkState() != Qt::Checked) {
+    m_ui->allowButton->setDefault(!selectedRows.isEmpty());
-            notSelected.append(item);
+    m_ui->allowButton->setAutoDefault(!selectedRows.isEmpty());
+
+    if (selectedRows.isEmpty()) {
+        m_ui->allowButton->clearFocus();
+        m_ui->denyButton->setFocus();
+    }
+}
+
+bool BrowserAccessControlDialog::areAllDisabled() const
+{
+    auto areAllDisabled = true;
+    for (const auto& item : getAllItems()) {
+        if (item->flags() != Qt::NoItemFlags) {
+            areAllDisabled = false;
         }
     }
-    return notSelected;
+
+    return areAllDisabled;
+}
+
+QList<QTableWidgetItem*> BrowserAccessControlDialog::getAllItems() const
+{
+    QList<QTableWidgetItem*> items;
+    for (int i = 0; i < m_ui->itemsTable->rowCount(); ++i) {
+        auto item = m_ui->itemsTable->item(i, 0);
+        items.append(item);
+    }
+    return items;
 }

src/browser/BrowserAccessControlDialog.h

@@ -1,6 +1,6 @@
 /*
+ *  Copyright (C) 2023 KeePassXC Team <team@keepassxc.org>
  *  Copyright (C) 2013 Francois Ferrand
- *  Copyright (C) 2022 KeePassXC Team <team@keepassxc.org>
  *
  *  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
@@ -29,6 +29,13 @@ namespace Ui
     class BrowserAccessControlDialog;
 }
 
+enum SelectionType
+{
+    Selected,
+    NonSelected,
+    Disabled
+};
+
 class BrowserAccessControlDialog : public QDialog
 {
     Q_OBJECT
@@ -37,20 +44,24 @@ public:
     explicit BrowserAccessControlDialog(QWidget* parent = nullptr);
     ~BrowserAccessControlDialog() override;
 
-    void setItems(const QList<Entry*>& items, const QString& urlString, bool httpAuth);
+    void setEntries(const QList<Entry*>& entriesToConfirm, const QString& urlString, bool httpAuth);
     bool remember() const;
-
+    QList<QTableWidgetItem*> getEntries(SelectionType selectionType) const;
-    QList<QTableWidgetItem*> getSelectedEntries() const;
-    QList<QTableWidgetItem*> getNonSelectedEntries() const;
 
 signals:
     void disableAccess(QTableWidgetItem* item);
 
+private slots:
+    void selectionChanged();
+
+private:
+    void addEntryToList(Entry* entry, int row);
+    bool areAllDisabled() const;
+    QList<QTableWidgetItem*> getAllItems() const;
+
 private:
     QScopedPointer<Ui::BrowserAccessControlDialog> m_ui;
     QList<Entry*> m_entriesToConfirm;
-    QList<Entry*> m_allowedEntries;
-    bool m_entriesAccepted;
 };
 
 #endif // KEEPASSXC_BROWSERACCESSCONTROLDIALOG_H

src/browser/BrowserAccessControlDialog.ui

@@ -31,7 +31,7 @@
     </widget>
    </item>
    <item>
-    <widget class="QTableWidget" name="itemsTable">
+    <widget class="CustomTableWidget" name="itemsTable">
      <property name="editTriggers">
       <set>QAbstractItemView::NoEditTriggers</set>
      </property>
@@ -39,7 +39,10 @@
       <bool>false</bool>
      </property>
      <property name="selectionMode">
-      <enum>QAbstractItemView::NoSelection</enum>
+      <enum>QAbstractItemView::ExtendedSelection</enum>
+     </property>
+     <property name="selectionBehavior">
+      <enum>QAbstractItemView::SelectRows</enum>
      </property>
      <property name="cornerButtonEnabled">
       <bool>false</bool>
@@ -110,6 +113,19 @@
    </item>
   </layout>
  </widget>
+ <tabstops>
+  <tabstop>rememberDecisionCheckBox</tabstop>
+  <tabstop>allowButton</tabstop>
+  <tabstop>denyButton</tabstop>
+ </tabstops>
+ <customwidgets>
+  <customwidget>
+   <class>CustomTableWidget</class>
+   <extends>QTableWidget</extends>
+   <header>browser/CustomTableWidget.h</header>
+   <container>1</container>
+  </customwidget>
+ </customwidgets>
  <resources/>
  <connections/>
 </ui>

src/browser/BrowserService.cpp

@@ -431,25 +431,48 @@ QList<Entry*> BrowserService::confirmEntries(QList<Entry*>& entriesToConfirm,
         denyEntry(entry, siteHost, formUrl, entryParameters.realm);
     });
 
-    accessControlDialog.setItems(entriesToConfirm, entryParameters.siteUrl, httpAuth);
+    accessControlDialog.setEntries(entriesToConfirm, entryParameters.siteUrl, httpAuth);
 
     QList<Entry*> allowedEntries;
     auto ret = accessControlDialog.exec();
-    for (auto item : accessControlDialog.getSelectedEntries()) {
+    auto remember = accessControlDialog.remember();
-        auto entry = entriesToConfirm[item->row()];
+
-        if (accessControlDialog.remember()) {
+    // All are denied
-            if (ret == QDialog::Accepted) {
+    if (ret == QDialog::Rejected && remember) {
+        for (auto& entry : entriesToConfirm) {
+            denyEntry(entry, siteHost, formUrl, entryParameters.realm);
+        }
+    }
+
+    // Some/all are accepted
+    if (ret == QDialog::Accepted) {
+        auto selectedEntries = accessControlDialog.getEntries(SelectionType::Selected);
+        for (auto& item : selectedEntries) {
+            auto entry = entriesToConfirm[item->row()];
+            allowedEntries.append(entry);
+
+            if (remember) {
                 allowEntry(entry, siteHost, formUrl, entryParameters.realm);
-            } else {
-                denyEntry(entry, siteHost, formUrl, entryParameters.realm);
             }
         }
 
-        if (ret == QDialog::Accepted) {
+        // Remembered non-selected entries must be denied
-            allowedEntries.append(entry);
+        if (remember) {
+            auto nonSelectedEntries = accessControlDialog.getEntries(SelectionType::NonSelected);
+            for (auto& item : nonSelectedEntries) {
+                auto entry = entriesToConfirm[item->row()];
+                denyEntry(entry, siteHost, formUrl, entryParameters.realm);
+            }
         }
     }
 
+    // Handle disabled entries (returned Accept/Reject status does not matter)
+    auto disabledEntries = accessControlDialog.getEntries(SelectionType::Disabled);
+    for (auto& item : disabledEntries) {
+        auto entry = entriesToConfirm[item->row()];
+        denyEntry(entry, siteHost, formUrl, entryParameters.realm);
+    }
+
     // Re-hide the application if it wasn't visible before
     hideWindow();
     m_dialogActive = false;

src/browser/CMakeLists.txt

@@ -28,7 +28,9 @@ if(WITH_XC_BROWSER)
             BrowserService.cpp
             BrowserSettings.cpp
             BrowserShared.cpp
-            NativeMessageInstaller.cpp)
+            CustomTableWidget.cpp
+            NativeMessageInstaller.cpp
+            )
 
     if(WITH_XC_BROWSER_PASSKEYS)
         list(APPEND keepassxcbrowser_SOURCES

src/browser/CustomTableWidget.cpp

@@ -0,0 +1,42 @@
+/*
+ *  Copyright (C) 2023 KeePassXC Team <team@keepassxc.org>
+ *
+ *  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
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "CustomTableWidget.h"
+
+CustomTableWidget::CustomTableWidget(QWidget* parent)
+    : QTableWidget(parent)
+{
+}
+
+void CustomTableWidget::keyPressEvent(QKeyEvent* event)
+{
+    if ((event->key() == Qt::Key_Enter || event->key() == Qt::Key_Return) && !selectedItems().isEmpty()) {
+        emit acceptSelections();
+    } else {
+        QTableView::keyPressEvent(event);
+    }
+}
+
+void CustomTableWidget::focusInEvent(QFocusEvent* event)
+{
+    // For some reason accept button gets selected if table is clicked without any
+    // selections, even if the button is actually disabled. Connecting to this
+    // signal and adjusting the button focuses fixes the issue.
+    if (event->reason() == Qt::MouseFocusReason && selectedItems().isEmpty()) {
+        emit focusInWithoutSelections();
+    }
+}

src/browser/CustomTableWidget.h

@@ -0,0 +1,41 @@
+/*
+ *  Copyright (C) 2023 KeePassXC Team <team@keepassxc.org>
+ *
+ *  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
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef CUSTOMTABLEWIDGET_H
+#define CUSTOMTABLEWIDGET_H
+
+#include <QFocusEvent>
+#include <QKeyEvent>
+#include <QTableWidget>
+
+class CustomTableWidget : public QTableWidget
+{
+    Q_OBJECT
+
+public:
+    CustomTableWidget(QWidget* parent);
+
+signals:
+    void acceptSelections();
+    void focusInWithoutSelections();
+
+protected:
+    void keyPressEvent(QKeyEvent* event) override;
+    void focusInEvent(QFocusEvent* event) override;
+};
+
+#endif // CUSTOMTABLEWIDGET_H