FdoSecrest: allow remember decision for future entries

Also added a reset decision button in session management tab

Fixes #7464

* Fix distorted button in settings page: the default margin in QToolBar is too large for our use case in a table row.

share/translations/keepassxc_en.ts

@@ -78,10 +78,6 @@
         <source>Command Line</source>
         <translation type="unfinished"></translation>
     </message>
-    <message>
-        <source>Your decision for above entries will be remembered for the duration the requesting client is running.</source>
-        <translation type="unfinished"></translation>
-    </message>
     <message>
         <source>Details</source>
         <translation type="unfinished"></translation>
@@ -95,7 +91,15 @@
         <translation type="unfinished"></translation>
     </message>
     <message>
-        <source>Deny All</source>
+        <source>Your decision will be remembered for the duration while both the requesting client AND KeePassXC are running.</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Deny All &amp;&amp; Future</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Allow All &amp;&amp; &amp;Future</source>
         <translation type="unfinished"></translation>
     </message>
 </context>
@@ -5398,6 +5402,14 @@ We recommend you use the AppImage available on our downloads page.</source>
         <source>Disconnect this application</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Reset</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Reset any remembered decisions for this application</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>Merger</name>

src/fdosecrets/dbus/DBusClient.cpp

@@ -73,8 +73,8 @@ namespace FdoSecrets
 
     bool DBusClient::itemKnown(const QUuid& uuid) const
     {
-        return m_authorizedAll || m_allowed.contains(uuid) || m_allowedOnce.contains(uuid) || m_denied.contains(uuid)
+        return m_authorizedAll != AuthDecision::Undecided || m_allowed.contains(uuid) || m_allowedOnce.contains(uuid)
-               || m_deniedOnce.contains(uuid);
+               || m_denied.contains(uuid) || m_deniedOnce.contains(uuid);
     }
 
     bool DBusClient::itemAuthorized(const QUuid& uuid) const
@@ -83,10 +83,16 @@ namespace FdoSecrets
             // everyone is authorized if this is not enabled
             return true;
         }
-        if (m_authorizedAll) {
+
-            // this client is trusted
+        // check if we have catch-all decision
+        if (m_authorizedAll == AuthDecision::Allowed) {
             return true;
         }
+        if (m_authorizedAll == AuthDecision::Denied) {
+            return false;
+        }
+
+        // individual decisions
         if (m_deniedOnce.contains(uuid) || m_denied.contains(uuid)) {
             // explicitly denied
             return false;
@@ -132,14 +138,21 @@ namespace FdoSecrets
         }
     }
 
-    void DBusClient::setAllAuthorized(bool authorized)
+    void DBusClient::setAllAuthorized(AuthDecision authorized)
     {
+        // once variants doesn't make sense here
+        if (authorized == AuthDecision::AllowedOnce) {
+            authorized = AuthDecision::Allowed;
+        }
+        if (authorized == AuthDecision::DeniedOnce) {
+            authorized = AuthDecision::Denied;
+        }
         m_authorizedAll = authorized;
     }
 
     void DBusClient::clearAuthorization()
     {
-        m_authorizedAll = false;
+        m_authorizedAll = AuthDecision::Undecided;
         m_allowed.clear();
         m_allowedOnce.clear();
         m_denied.clear();

src/fdosecrets/dbus/DBusClient.h

@@ -159,7 +159,7 @@ namespace FdoSecrets
         /**
          * Authorize client to access all items.
          */
-        void setAllAuthorized(bool authorized = true);
+        void setAllAuthorized(AuthDecision authorized);
 
         /**
          * Forget all previous authorization.
@@ -176,7 +176,7 @@ namespace FdoSecrets
         QPointer<DBusMgr> m_dbus;
         PeerInfo m_process;
 
-        bool m_authorizedAll{false};
+        AuthDecision m_authorizedAll{AuthDecision::Undecided};
 
         QSet<QUuid> m_allowed{};
         QSet<QUuid> m_denied{};

src/fdosecrets/objects/Prompt.cpp

@@ -303,7 +303,7 @@ namespace FdoSecrets
                     }
                     continue;
                 }
-                // attach a temporary property so later we can get the item
+                // attach a temporary property, so later we can get the item
                 // back from the dialog's result
                 entry->setProperty(FdoSecretsBackend, QVariant::fromValue(item.data()));
                 entries << entry;
@@ -317,11 +317,11 @@ namespace FdoSecrets
             connect(ac, &AccessControlDialog::finished, ac, &AccessControlDialog::deleteLater);
             ac->open();
         } else {
-            itemUnlockFinished({});
+            itemUnlockFinished({}, AuthDecision::Undecided);
         }
     }
 
-    void UnlockPrompt::itemUnlockFinished(const QHash<Entry*, AuthDecision>& decisions)
+    void UnlockPrompt::itemUnlockFinished(const QHash<Entry*, AuthDecision>& decisions, AuthDecision forFutureEntries)
     {
         auto client = m_client.lock();
         if (!client) {
@@ -345,6 +345,9 @@ namespace FdoSecrets
                 m_numRejected += 1;
             }
         }
+        if (forFutureEntries != AuthDecision::Undecided) {
+            client->setAllAuthorized(forFutureEntries);
+        }
         // if anything is not unlocked, treat the whole prompt as dismissed
         // so the client has a chance to handle the error
         finishPrompt(m_numRejected > 0);

src/fdosecrets/objects/Prompt.h

@@ -169,7 +169,7 @@ namespace FdoSecrets
         QVariant currentResult() const override;
 
         void collectionUnlockFinished(bool accepted);
-        void itemUnlockFinished(const QHash<Entry*, AuthDecision>& results);
+        void itemUnlockFinished(const QHash<Entry*, AuthDecision>& results, AuthDecision forFutureEntries);
         void unlockItems();
 
         static constexpr auto FdoSecretsBackend = "FdoSecretsBackend";

src/fdosecrets/widgets/AccessControlDialog.cpp

@@ -68,8 +68,6 @@ AccessControlDialog::AccessControlDialog(QWindow* parent,
     m_ui->itemsTable->resizeColumnsToContents();
 
     // the info widget
-    m_ui->rememberMsg->setMessageType(MessageWidget::Information);
-    m_ui->rememberMsg->show(); // sync with m_rememberCheck->setChecked(true)
     m_ui->exePathWarn->setMessageType(MessageWidget::Warning);
     m_ui->exePathWarn->hide();
 
@@ -81,19 +79,28 @@ AccessControlDialog::AccessControlDialog(QWindow* parent,
         m_ui->buttonBox->addButton(detailsButtonText + QStringLiteral(" >>"), QDialogButtonBox::HelpRole);
     detailsButton->setCheckable(true);
 
+    QString tooltip = QStringLiteral("<p align='justify'>%1</p>")
+                          .arg(tr("Your decision will be remembered for the duration while both the requesting client "
+                                  "AND KeePassXC are running."));
+
     m_rememberCheck = new QCheckBox(tr("Remember"), this);
     m_rememberCheck->setObjectName("rememberCheck"); // for testing
     m_rememberCheck->setChecked(true);
     m_ui->buttonBox->addButton(m_rememberCheck, QDialogButtonBox::ActionRole);
+    m_rememberCheck->setToolTip(tooltip);
 
     auto allowButton = m_ui->buttonBox->addButton(tr("Allow Selected"), QDialogButtonBox::AcceptRole);
     allowButton->setDefault(true);
 
-    auto cancelButton = m_ui->buttonBox->addButton(tr("Deny All"), QDialogButtonBox::RejectRole);
+    auto cancelButton = m_ui->buttonBox->addButton(tr("Deny All && Future"), QDialogButtonBox::RejectRole);
+    cancelButton->setToolTip(tooltip);
+
+    auto allowAllButton = m_ui->buttonBox->addButton(tr("Allow All && &Future"), QDialogButtonBox::AcceptRole);
+    allowAllButton->setToolTip(tooltip);
 
     connect(cancelButton, &QPushButton::clicked, this, [this]() { done(DenyAll); });
     connect(allowButton, &QPushButton::clicked, this, [this]() { done(AllowSelected); });
-    connect(m_rememberCheck, &QCheckBox::clicked, this, &AccessControlDialog::rememberChecked);
+    connect(allowAllButton, &QPushButton::clicked, this, [this]() { done(AllowAll); });
     connect(detailsButton, &QPushButton::clicked, this, [=](bool checked) {
         m_ui->detailsContainer->setVisible(checked);
         if (checked) {
@@ -119,6 +126,9 @@ AccessControlDialog::AccessControlDialog(QWindow* parent,
         detailsButton->click();
     }
 
+    // adjust size after we initialize the button box
+    adjustSize();
+
     allowButton->setFocus();
 }
 
@@ -146,15 +156,6 @@ void AccessControlDialog::setupDetails(const FdoSecrets::PeerInfo& info)
     m_ui->detailsContainer->hide();
 }
 
-void AccessControlDialog::rememberChecked(bool checked)
-{
-    if (checked) {
-        m_ui->rememberMsg->animatedShow();
-    } else {
-        m_ui->rememberMsg->animatedHide();
-    }
-}
-
 void AccessControlDialog::denyEntryClicked(Entry* entry, const QModelIndex& index)
 {
     m_decisions.insert(entry, AuthDecision::Denied);
@@ -166,32 +167,35 @@ void AccessControlDialog::denyEntryClicked(Entry* entry, const QModelIndex& inde
 
 void AccessControlDialog::dialogFinished(int result)
 {
-    auto allow = m_rememberCheck->isChecked() ? AuthDecision::Allowed : AuthDecision::AllowedOnce;
+    auto decision = AuthDecision::Undecided;
-    auto deny = m_rememberCheck->isChecked() ? AuthDecision::Denied : AuthDecision::DeniedOnce;
+    auto futureDecision = AuthDecision::Undecided;
+    switch (result) {
+    case AllowSelected:
+        decision = m_rememberCheck->isChecked() ? AuthDecision::Allowed : AuthDecision::AllowedOnce;
+        break;
+    case AllowAll:
+        decision = AuthDecision::Allowed;
+        futureDecision = AuthDecision::Allowed;
+        break;
+    case DenyAll:
+        decision = AuthDecision::Denied;
+        futureDecision = AuthDecision::Denied;
+        break;
+    case Rejected:
+    default:
+        break;
+    }
 
     for (int row = 0; row != m_model->rowCount({}); ++row) {
         auto entry = m_model->data(m_model->index(row, 2), Qt::EditRole).value<Entry*>();
         auto selected = m_model->data(m_model->index(row, 0), Qt::CheckStateRole).value<Qt::CheckState>();
         Q_ASSERT(entry);
-        switch (result) {
+
-        case AllowSelected:
+        auto undecided = result == AllowSelected && !selected;
-            if (selected) {
+        m_decisions.insert(entry, undecided ? AuthDecision::Undecided : decision);
-                m_decisions.insert(entry, allow);
-            } else {
-                m_decisions.insert(entry, AuthDecision::Undecided);
-            }
-            break;
-        case DenyAll:
-            m_decisions.insert(entry, deny);
-            break;
-        case Rejected:
-        default:
-            m_decisions.insert(entry, AuthDecision::Undecided);
-            break;
-        }
     }
 
-    emit finished(m_decisions);
+    emit finished(m_decisions, futureDecision);
 }
 
 QHash<Entry*, AuthDecision> AccessControlDialog::decisions() const

src/fdosecrets/widgets/AccessControlDialog.h

@@ -66,16 +66,16 @@ public:
     {
         Rejected,
         AllowSelected,
+        AllowAll,
         DenyAll,
     };
 
     QHash<Entry*, AuthDecision> decisions() const;
 
 signals:
-    void finished(const QHash<Entry*, AuthDecision>& results);
+    void finished(const QHash<Entry*, AuthDecision>& results, AuthDecision forFutureEntries);
 
 private slots:
-    void rememberChecked(bool checked);
     void denyEntryClicked(Entry* entry, const QModelIndex& index);
     void dialogFinished(int result);
 

src/fdosecrets/widgets/AccessControlDialog.ui

@@ -122,16 +122,6 @@
      </attribute>
     </widget>
    </item>
-   <item>
-    <widget class="MessageWidget" name="rememberMsg" native="true">
-     <property name="text" stdset="0">
-      <string>Your decision for above entries will be remembered for the duration the requesting client is running.</string>
-     </property>
-     <property name="closeButtonVisible" stdset="0">
-      <bool>false</bool>
-     </property>
-    </widget>
-   </item>
    <item>
     <layout class="QHBoxLayout" name="horizontalLayout">
      <item>

src/fdosecrets/widgets/SettingsWidgetFdoSecrets.cpp

@@ -26,7 +26,8 @@
 
 #include "gui/DatabaseWidget.h"
 
-#include <QToolBar>
+#include <QAction>
+#include <QToolButton>
 
 using FdoSecrets::DBusClientPtr;
 using FdoSecrets::SettingsClientModel;
@@ -34,7 +35,7 @@ using FdoSecrets::SettingsDatabaseModel;
 
 namespace
 {
-    class ManageDatabase : public QToolBar
+    class ManageDatabase : public QWidget
     {
         Q_OBJECT
 
@@ -42,18 +43,9 @@ namespace
 
     public:
         explicit ManageDatabase(FdoSecretsPlugin* plugin, QWidget* parent = nullptr)
-            : QToolBar(parent)
+            : QWidget(parent)
             , m_plugin(plugin)
         {
-            setFloatable(false);
-            setMovable(false);
-
-            // use a dummy widget to center the buttons
-            auto spacer = new QWidget(this);
-            spacer->setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Preferred);
-            spacer->setVisible(true);
-            addWidget(spacer);
-
             // db settings
             m_dbSettingsAct = new QAction(tr("Database settings"), this);
             m_dbSettingsAct->setIcon(icons()->icon(QStringLiteral("document-edit")));
@@ -63,14 +55,12 @@ namespace
                 if (!m_dbWidget) {
                     return;
                 }
-                auto db = m_dbWidget;
                 m_plugin->serviceInstance()->doSwitchToDatabaseSettings(m_dbWidget);
             });
-            addAction(m_dbSettingsAct);
 
             // unlock/lock
             m_lockAct = new QAction(tr("Unlock database"), this);
-            m_lockAct->setIcon(icons()->icon(QStringLiteral("object-locked")));
+            m_lockAct->setIcon(icons()->icon(QStringLiteral("object-unlocked")));
             m_lockAct->setToolTip(tr("Unlock database to show more information"));
             connect(m_lockAct, &QAction::triggered, this, [this]() {
                 if (!m_dbWidget) {
@@ -83,13 +73,23 @@ namespace
                 }
             });
 
-            addAction(m_lockAct);
+            // layout
+            auto dbSettingsBtn = new QToolButton(this);
+            dbSettingsBtn->setAutoRaise(true);
+            dbSettingsBtn->setDefaultAction(m_dbSettingsAct);
 
-            // use a dummy widget to center the buttons
+            auto lockBtn = new QToolButton(this);
-            spacer = new QWidget(this);
+            lockBtn->setAutoRaise(true);
-            spacer->setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Preferred);
+            lockBtn->setDefaultAction(m_lockAct);
-            spacer->setVisible(true);
+
-            addWidget(spacer);
+            auto layout = new QHBoxLayout(this);
+            layout->setContentsMargins(1, 1, 1, 1);
+            layout->setSpacing(1);
+
+            layout->addStretch();
+            layout->addWidget(dbSettingsBtn);
+            layout->addWidget(lockBtn);
+            layout->addStretch();
         }
 
         DatabaseWidget* dbWidget() const
@@ -127,14 +127,20 @@ namespace
                 return;
             }
             connect(m_dbWidget, &DatabaseWidget::databaseLocked, this, [this]() {
+                if (!m_lockAct || !m_dbSettingsAct) {
+                    return;
+                }
                 m_lockAct->setText(tr("Unlock database"));
-                m_lockAct->setIcon(icons()->icon(QStringLiteral("object-locked")));
+                m_lockAct->setIcon(icons()->icon(QStringLiteral("object-unlocked")));
                 m_lockAct->setToolTip(tr("Unlock database to show more information"));
                 m_dbSettingsAct->setEnabled(false);
             });
             connect(m_dbWidget, &DatabaseWidget::databaseUnlocked, this, [this]() {
+                if (!m_lockAct || !m_dbSettingsAct) {
+                    return;
+                }
                 m_lockAct->setText(tr("Lock database"));
-                m_lockAct->setIcon(icons()->icon(QStringLiteral("object-unlocked")));
+                m_lockAct->setIcon(icons()->icon(QStringLiteral("object-locked")));
                 m_lockAct->setToolTip(tr("Lock database"));
                 m_dbSettingsAct->setEnabled(true);
             });
@@ -147,7 +153,7 @@ namespace
         QAction* m_lockAct = nullptr;
     };
 
-    class ManageSession : public QToolBar
+    class ManageSession : public QWidget
     {
         Q_OBJECT
 
@@ -155,17 +161,8 @@ namespace
 
     public:
         explicit ManageSession(QWidget* parent = nullptr)
-            : QToolBar(parent)
+            : QWidget(parent)
         {
-            setFloatable(false);
-            setMovable(false);
-
-            // use a dummy widget to center the buttons
-            auto spacer = new QWidget(this);
-            spacer->setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Preferred);
-            spacer->setVisible(true);
-            addWidget(spacer);
-
             auto disconnectAct = new QAction(tr("Disconnect"), this);
             disconnectAct->setIcon(icons()->icon(QStringLiteral("dialog-close")));
             disconnectAct->setToolTip(tr("Disconnect this application"));
@@ -174,13 +171,33 @@ namespace
                     m_client->disconnectDBus();
                 }
             });
-            addAction(disconnectAct);
 
-            // use a dummy widget to center the buttons
+            auto resetAct = new QAction(tr("Reset"), this);
-            spacer = new QWidget(this);
+            resetAct->setIcon(icons()->icon(QStringLiteral("refresh")));
-            spacer->setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Preferred);
+            resetAct->setToolTip(tr("Reset any remembered decisions for this application"));
-            spacer->setVisible(true);
+            connect(resetAct, &QAction::triggered, this, [this]() {
-            addWidget(spacer);
+                if (m_client) {
+                    m_client->clearAuthorization();
+                }
+            });
+
+            // layout
+            auto disconnectBtn = new QToolButton(this);
+            disconnectBtn->setAutoRaise(true);
+            disconnectBtn->setDefaultAction(disconnectAct);
+
+            auto resetBtn = new QToolButton(this);
+            resetBtn->setAutoRaise(true);
+            resetBtn->setDefaultAction(resetAct);
+
+            auto layout = new QHBoxLayout(this);
+            layout->setContentsMargins(1, 1, 1, 1);
+            layout->setSpacing(1);
+
+            layout->addStretch();
+            layout->addWidget(resetBtn);
+            layout->addWidget(disconnectBtn);
+            layout->addStretch();
         }
 
         const DBusClientPtr& client() const

tests/gui/TestGuiFdoSecrets.cpp

@@ -608,6 +608,62 @@ void TestGuiFdoSecrets::testServiceUnlockItems()
     DBUS_COMPARE(item->locked(), false);
 }
 
+void TestGuiFdoSecrets::testServiceUnlockItemsIncludeFutureEntries()
+{
+    FdoSecrets::settings()->setConfirmAccessItem(true);
+
+    auto service = enableService();
+    VERIFY(service);
+    auto coll = getDefaultCollection(service);
+    VERIFY(coll);
+    auto item = getFirstItem(coll);
+    VERIFY(item);
+    auto sess = openSession(service, DhIetf1024Sha256Aes128CbcPkcs7::Algorithm);
+    VERIFY(sess);
+
+    DBUS_COMPARE(item->locked(), true);
+
+    {
+        DBUS_GET2(unlocked, promptPath, service->Unlock({QDBusObjectPath(item->path())}));
+        // nothing is unlocked immediately without user's action
+        COMPARE(unlocked, {});
+
+        auto prompt = getProxy<PromptProxy>(promptPath);
+        VERIFY(prompt);
+        QSignalSpy spyPromptCompleted(prompt.data(), SIGNAL(Completed(bool, QDBusVariant)));
+        VERIFY(spyPromptCompleted.isValid());
+
+        // nothing is unlocked yet
+        COMPARE(spyPromptCompleted.count(), 0);
+        DBUS_COMPARE(item->locked(), true);
+
+        // drive the prompt
+        DBUS_VERIFY(prompt->Prompt(""));
+        // remember and include future entries
+        VERIFY(driveAccessControlDialog(true, true));
+
+        VERIFY(waitForSignal(spyPromptCompleted, 1));
+        {
+            auto args = spyPromptCompleted.takeFirst();
+            COMPARE(args.size(), 2);
+            COMPARE(args.at(0).toBool(), false);
+            COMPARE(getSignalVariantArgument<QList<QDBusObjectPath>>(args.at(1)), {QDBusObjectPath(item->path())});
+        }
+
+        // unlocked
+        DBUS_COMPARE(item->locked(), false);
+    }
+
+    // check other entries are also unlocked
+    {
+        DBUS_GET(itemPaths, coll->items());
+        VERIFY(itemPaths.size() > 1);
+        auto anotherItem = getProxy<ItemProxy>(itemPaths.last());
+        VERIFY(anotherItem);
+        DBUS_COMPARE(anotherItem->locked(), false);
+    }
+}
+
 void TestGuiFdoSecrets::testServiceLock()
 {
     auto service = enableService();
@@ -1635,7 +1691,7 @@ TestGuiFdoSecrets::encryptPassword(QByteArray value, QString contentType, const
     return m_clientCipher->encrypt(ss.unmarshal(m_plugin->dbus())).marshal();
 }
 
-bool TestGuiFdoSecrets::driveAccessControlDialog(bool remember)
+bool TestGuiFdoSecrets::driveAccessControlDialog(bool remember, bool includeFutureEntries)
 {
     processEvents();
     for (auto w : QApplication::topLevelWidgets()) {
@@ -1647,7 +1703,13 @@ bool TestGuiFdoSecrets::driveAccessControlDialog(bool remember)
             auto rememberCheck = dlg->findChild<QCheckBox*>("rememberCheck");
             VERIFY(rememberCheck);
             rememberCheck->setChecked(remember);
-            dlg->done(AccessControlDialog::AllowSelected);
+
+            if (includeFutureEntries) {
+                dlg->done(AccessControlDialog::AllowAll);
+            } else {
+                dlg->done(AccessControlDialog::AllowSelected);
+            }
+
             processEvents();
             VERIFY(dlg->isHidden());
             return true;

tests/gui/TestGuiFdoSecrets.h

@@ -71,6 +71,7 @@ private slots:
     void testServiceUnlock();
     void testServiceUnlockDatabaseConcurrent();
     void testServiceUnlockItems();
+    void testServiceUnlockItemsIncludeFutureEntries();
     void testServiceLock();
     void testServiceLockConcurrent();
 
@@ -103,7 +104,7 @@ private slots:
 private:
     bool driveUnlockDialog();
     bool driveNewDatabaseWizard();
-    bool driveAccessControlDialog(bool remember = true);
+    bool driveAccessControlDialog(bool remember = true, bool includeFutureEntries = false);
     bool waitForSignal(QSignalSpy& spy, int expectedCount);
 
     void processEvents();