fix(key-server): handle missing keys consistently

- normalize 404 and empty responses as plain not-found results
- distinguish missing keys from transport failures in publication checks
- add key-server not-found helpers and unit coverage
- disable batch actions for key groups and empty targets
- add publish and refresh actions to the key manager menu
- reformat affected C++ declarations and callbacks

6 files changed, 170 insertions, 95 deletions

diff --git a/src/m_key_server_sync/KeyServerBatchLogic.cpp b/src/m_key_server_sync/KeyServerBatchLogic.cpp
index c67f65e..02b93fc 100644
--- a/src/m_key_server_sync/KeyServerBatchLogic.cpp
+++ b/src/m_key_server_sync/KeyServerBatchLogic.cpp
@@ -73,6 +73,15 @@ auto JoinForImport(const QList<QByteArray>& blocks) -> QByteArray {
   return out;
 }
 
+auto NotFoundText() -> QString {
+  return QCoreApplication::translate("GTrC",
+                                     "The key server does not have this key.");
+}
+
+auto IsNotFound(const QString& error) -> bool {
+  return error == NotFoundText();
+}
+
 auto RefreshSummary(int fetched, int total, const QStringList& failures)
     -> QString {
   return WithFailures(
diff --git a/src/m_key_server_sync/KeyServerBatchLogic.h b/src/m_key_server_sync/KeyServerBatchLogic.h
index 439ef87..60ea682 100644
--- a/src/m_key_server_sync/KeyServerBatchLogic.h
+++ b/src/m_key_server_sync/KeyServerBatchLogic.h
@@ -68,6 +68,13 @@ auto Normalise(const std::optional<BatchKey>& key,
  */
 auto JoinForImport(const QList<QByteArray>& blocks) -> QByteArray;
 
+/// What a lookup failure says when the server simply does not have the key:
+/// not an error, and never the transport's own wording.
+auto NotFoundText() -> QString;
+
+/// Whether @p error is NotFoundText().
+auto IsNotFound(const QString& error) -> bool;
+
 /// What a finished refresh says, e.g. "3 of 4 keys fetched" plus failures.
 auto RefreshSummary(int fetched, int total, const QStringList& failures)
     -> QString;
diff --git a/src/m_key_server_sync/KeyServerSyncModule.cpp b/src/m_key_server_sync/KeyServerSyncModule.cpp
index 0c4122b..aae1387 100644
--- a/src/m_key_server_sync/KeyServerSyncModule.cpp
+++ b/src/m_key_server_sync/KeyServerSyncModule.cpp
@@ -90,8 +90,9 @@ using ErrorCallback = std::function<void(const QString&, const QString&)>;
 void FetchKey(const KeyServerList::Route& route, const QString& handle,
               bool by_fingerprint, const KeyCallback& on_key,
               const ErrorCallback& on_error, QObject* owner = nullptr) {
-  QObject* context =
-      owner != nullptr ? owner : static_cast<QObject*>(QThread::currentThread());
+  QObject* context = owner != nullptr
+                         ? owner
+                         : static_cast<QObject*>(QThread::currentThread());
   if (route.vks) {
     auto* vks = new VKSInterface(route.url, owner);
     QObject::connect(vks, &VKSInterface::SignalKeyRetrieved, context,
@@ -118,16 +119,16 @@ void FetchKey(const KeyServerList::Route& route, const QString& handle,
       pks, &PKSInterface::SignalKeyServerKeyLookupResult, context,
       [on_key, on_error](QNetworkReply::NetworkError error,
                          const QString& error_string, const QByteArray& data) {
-        if (error != QNetworkReply::NoError) {
-          on_error(error_string, QString::fromUtf8(data));
+        // A 404, or the empty 200 some HKP servers answer instead, means
+        // "no such key": importing an empty answer would report success
+        // while changing nothing.
+        if (error == QNetworkReply::ContentNotFoundError ||
+            (error == QNetworkReply::NoError && data.trimmed().isEmpty())) {
+          on_error(KeyServerBatchLogic::NotFoundText(), {});
           return;
         }
-        // An empty 200 is how some HKP servers say "no such key"; importing it
-        // would report success while changing nothing.
-        if (data.trimmed().isEmpty()) {
-          on_error(QCoreApplication::translate(
-                       "GTrC", "The key server did not return a key."),
-                   {});
+        if (error != QNetworkReply::NoError) {
+          on_error(error_string, QString::fromUtf8(data));
           return;
         }
         on_key(QString::fromUtf8(data));
@@ -186,7 +187,8 @@ class KeyServerBatch : public QObject {
   static constexpr int kSpacingMs = 200;
 
   KeyServerBatch(Kind kind, QList<BatchKey> keys)
-      : kind_(kind), keys_(std::move(keys)),
+      : kind_(kind),
+        keys_(std::move(keys)),
         route_(KeyServerList::SyncRoute()) {}
 
   /// @return false when the user declined, and the batch is already gone
@@ -213,8 +215,8 @@ class KeyServerBatch : public QObject {
             Schedule();
           },
           [this, key](const QString& error, const QString&) {
-            failures_.append(QStringLiteral("%1: %2").arg(key.fingerprint,
-                                                          error));
+            failures_.append(
+                QStringLiteral("%1: %2").arg(key.fingerprint, error));
             Schedule();
           },
           this);
@@ -224,9 +226,8 @@ class KeyServerBatch : public QObject {
   }
 
   void Publish(const BatchKey& key) {
-    const auto exported =
-        gf::sdk::ExportKey(GFModuleSdkContext(),
-                           static_cast<int>(key.channel), key.key_id, true);
+    const auto exported = gf::sdk::ExportKey(
+        GFModuleSdkContext(), static_cast<int>(key.channel), key.key_id, true);
     if (exported.isEmpty()) {
       failures_.append(QCoreApplication::translate(
                            "GTrC", "%1: the public key could not be exported")
@@ -243,14 +244,12 @@ class KeyServerBatch : public QObject {
     };
     if (route_.vks) {
       auto* vks = new VKSInterface(route_.url, this);
-      connect(vks, &VKSInterface::SignalKeyUploaded, this,
-              [ok](const QString&, const QJsonObject&, const QString&) {
-                ok();
-              });
-      connect(vks, &VKSInterface::SignalErrorOccurred, this,
-              [failed](const QString& error, const QString&) {
-                failed(error);
-              });
+      connect(
+          vks, &VKSInterface::SignalKeyUploaded, this,
+          [ok](const QString&, const QJsonObject&, const QString&) { ok(); });
+      connect(
+          vks, &VKSInterface::SignalErrorOccurred, this,
+          [failed](const QString& error, const QString&) { failed(error); });
       vks->UploadKey(QString::fromUtf8(exported));
       return;
     }
@@ -281,14 +280,13 @@ class KeyServerBatch : public QObject {
       }
       Tell(severity,
            QCoreApplication::translate("GTrC", "Key Refresh Finished"),
-           KeyServerBatchLogic::RefreshSummary(
-               static_cast<int>(blocks_.size()), total, failures_));
+           KeyServerBatchLogic::RefreshSummary(static_cast<int>(blocks_.size()),
+                                               total, failures_));
     } else {
       Tell(severity,
            QCoreApplication::translate("GTrC", "Key Publishing Finished"),
-           KeyServerBatchLogic::PublishSummary(done_, total,
-                                               QUrl(route_.url).host(),
-                                               failures_));
+           KeyServerBatchLogic::PublishSummary(
+               done_, total, QUrl(route_.url).host(), failures_));
     }
     deleteLater();
   }
@@ -309,8 +307,9 @@ void StartBatch(KeyServerBatch::Kind kind, QList<BatchKey> keys) {
   if (!g_batch.isNull()) {
     Tell(Severity::kInfo, QCoreApplication::translate("GTrC", "Key Server"),
          QCoreApplication::translate(
-             "GTrC", "A key server operation is already running. Try again "
-                     "when it has finished."));
+             "GTrC",
+             "A key server operation is already running. Try again "
+             "when it has finished."));
     return;
   }
   auto* batch = new KeyServerBatch(kind, std::move(keys));
@@ -369,12 +368,13 @@ auto UploadKeyToServer(int channel, const QString& key_id) -> int {
   const auto exported =
       gf::sdk::ExportKey(GFModuleSdkContext(), channel, key_id, true);
   if (exported.isEmpty()) {
-    Tell(Severity::kError, QCoreApplication::translate("GTrC", "Key Upload Failed"),
-        QCoreApplication::translate(
-            "GTrC",
-            "Failed to export the public key before uploading.\n"
-            "Key: %1")
-            .arg(key_id));
+    Tell(Severity::kError,
+         QCoreApplication::translate("GTrC", "Key Upload Failed"),
+         QCoreApplication::translate(
+             "GTrC",
+             "Failed to export the public key before uploading.\n"
+             "Key: %1")
+             .arg(key_id));
     return -1;
   }
 
@@ -391,27 +391,29 @@ auto UploadKeyToServer(int channel, const QString& key_id) -> int {
         pks, &PKSInterface::SignalKeyServerKeyUploadResult,
         QThread::currentThread(),
         [server, key_id](QNetworkReply::NetworkError error,
-                                 const QString& error_string) {
+                         const QString& error_string) {
           if (error != QNetworkReply::NoError) {
-            Tell(Severity::kError, QCoreApplication::translate("GTrC", "Key Upload Failed"),
-                QCoreApplication::translate(
-                    "GTrC",
-                    "Failed to upload public key to the server.\n"
-                    "Fingerprint: %1\n"
-                    "Error: %2")
-                    .arg(key_id, error_string));
+            Tell(Severity::kError,
+                 QCoreApplication::translate("GTrC", "Key Upload Failed"),
+                 QCoreApplication::translate(
+                     "GTrC",
+                     "Failed to upload public key to the server.\n"
+                     "Fingerprint: %1\n"
+                     "Error: %2")
+                     .arg(key_id, error_string));
             return;
           }
 
           // No verification mail follows an HKP upload, so do not promise one.
-          Tell(Severity::kInfo, QCoreApplication::translate("GTrC",
-                                          "Public Key Upload Successful"),
-              QCoreApplication::translate(
-                  "GTrC",
-                  "The public key was uploaded to the key server %2 over "
-                  "HKP.\n"
-                  "Fingerprint: %1")
-                  .arg(key_id, QUrl(server).host()));
+          Tell(Severity::kInfo,
+               QCoreApplication::translate("GTrC",
+                                           "Public Key Upload Successful"),
+               QCoreApplication::translate(
+                   "GTrC",
+                   "The public key was uploaded to the key server %2 over "
+                   "HKP.\n"
+                   "Fingerprint: %1")
+                   .arg(key_id, QUrl(server).host()));
         });
     QObject::connect(pks, &PKSInterface::SignalKeyServerKeyUploadResult, pks,
                      &PKSInterface::deleteLater);
@@ -424,7 +426,7 @@ auto UploadKeyToServer(int channel, const QString& key_id) -> int {
   QObject::connect(
       vks, &VKSInterface::SignalKeyUploaded, QThread::currentThread(),
       [server](const QString& fpr, const QJsonObject& status,
-                       const QString& token) {
+               const QString& token) {
         // Handle successful response
         QString status_message = QCoreApplication::translate(
             "GTrC", "The following email addresses have status:\n");
@@ -445,7 +447,9 @@ auto UploadKeyToServer(int channel, const QString& key_id) -> int {
         const auto host = QUrl(server).host();
 
         // Notify user of successful upload and status details
-        Tell(Severity::kInfo, QCoreApplication::translate("GTrC", "Public Key Upload Successful"),
+        Tell(
+            Severity::kInfo,
+            QCoreApplication::translate("GTrC", "Public Key Upload Successful"),
             QCoreApplication::translate(
                 "GTrC",
                 "The public key was successfully uploaded to the "
@@ -460,13 +464,14 @@ auto UploadKeyToServer(int channel, const QString& key_id) -> int {
   QObject::connect(
       vks, &VKSInterface::SignalErrorOccurred, QThread::currentThread(),
       [key_id](const QString& error, const QString& data) {
-        Tell(Severity::kError, QCoreApplication::translate("GTrC", "Key Upload Failed"),
-            QCoreApplication::translate(
-                "GTrC",
-                "Failed to upload public key to the server.\n"
-                "Fingerprint: %1\n"
-                "Error: %2")
-                .arg(key_id, error));
+        Tell(Severity::kError,
+             QCoreApplication::translate("GTrC", "Key Upload Failed"),
+             QCoreApplication::translate(
+                 "GTrC",
+                 "Failed to upload public key to the server.\n"
+                 "Fingerprint: %1\n"
+                 "Error: %2")
+                 .arg(key_id, error));
       });
 
   QObject::connect(vks, &VKSInterface::SignalKeyUploaded, vks,
@@ -492,13 +497,14 @@ auto UpdateKeyFromKeyServer(int channel, const QString& fpr) -> int {
         Q_UNUSED(data);
         // Name the server: it is the user's choice now, and a failure they
         // cannot attribute to a host is one they cannot fix.
-        Tell(Severity::kError, QCoreApplication::translate("GTrC", "Key Update Failed"),
-            QCoreApplication::translate(
-                "GTrC",
-                "Failed to retrieve public key from %3.\n"
-                "Key ID: %1\n"
-                "Error: %2")
-                .arg(fpr, error, host));
+        Tell(Severity::kError,
+             QCoreApplication::translate("GTrC", "Key Update Failed"),
+             QCoreApplication::translate(
+                 "GTrC",
+                 "Failed to retrieve public key from %3.\n"
+                 "Key ID: %1\n"
+                 "Error: %2")
+                 .arg(fpr, error, host));
       });
   return 0;
 }
@@ -536,18 +542,24 @@ auto KeysOf(const KeyArgs& a) -> QList<BatchKey> {
 
 struct PublishKey {
   static constexpr gf::cmd::Meta kMeta{
-      GF_MODULE_ID ".publish_key", GC_TR("Publish Public Key to Key Server"),
+      GF_MODULE_ID ".publish_key",
+      GC_TR("Publish Public Key to Key Server"),
       GC_TR("Upload the public key to the key server used for syncing"),
-      GC_TR("Key Server Operations"), 0, gf::cmd::kNeedsGuiThread};
+      GC_TR("Key Server Operations"),
+      0,
+      gf::cmd::kNeedsGuiThread};
   using Args = KeyArgs;
   using Result = gf::cmd::Unit;
 };
 
 struct RefreshKey {
   static constexpr gf::cmd::Meta kMeta{
-      GF_MODULE_ID ".refresh_key", GC_TR("Refresh Public Key From Key Server"),
+      GF_MODULE_ID ".refresh_key",
+      GC_TR("Refresh Public Key From Key Server"),
       GC_TR("Import the latest copy of the public key from the key server"),
-      GC_TR("Key Server Operations"), 0, gf::cmd::kNeedsGuiThread};
+      GC_TR("Key Server Operations"),
+      0,
+      gf::cmd::kNeedsGuiThread};
   using Args = KeyArgs;
   using Result = gf::cmd::Unit;
 };
@@ -557,15 +569,20 @@ struct CheckPublication {
       GF_MODULE_ID ".check_publication",
       GC_TR("Check Publication Status"),
       GC_TR("Ask the key server whether it has this public key"),
-      GC_TR("Key Server Operations"), 0, gf::cmd::kNeedsGuiThread};
+      GC_TR("Key Server Operations"),
+      0,
+      gf::cmd::kNeedsGuiThread};
   using Args = KeyArgs;
   using Result = gf::cmd::Unit;
 };
 
 struct SearchKey {
   static constexpr gf::cmd::Meta kMeta{
-      GF_MODULE_ID ".search_key", GC_TR("Key Server"),
-      GC_TR("Import public keys from a trusted key server."), "", 0,
+      GF_MODULE_ID ".search_key",
+      GC_TR("Key Server"),
+      GC_TR("Import public keys from a trusted key server."),
+      "",
+      0,
       gf::cmd::kNeedsGuiThread};
   struct Args {
     std::optional<QString> fingerprint;
@@ -620,8 +637,7 @@ auto DoCheckPublication(const gf::cmd::CommandContext& /*ctx*/,
   const auto fpr = keys.front().fingerprint;
   const auto route = KeyServerList::SyncRoute();
   const auto host = QUrl(route.url).host();
-  const auto title =
-      QCoreApplication::translate("GTrC", "Publication Status");
+  const auto title = QCoreApplication::translate("GTrC", "Publication Status");
   FetchKey(
       route, fpr, true,
       [title, host](const QString&) {
@@ -631,9 +647,16 @@ auto DoCheckPublication(const gf::cmd::CommandContext& /*ctx*/,
                  .arg(host));
       },
       [title, host](const QString& error, const QString&) {
-        Tell(Severity::kInfo, title,
+        if (KeyServerBatchLogic::IsNotFound(error)) {
+          Tell(Severity::kInfo, title,
+               QCoreApplication::translate(
+                   "GTrC", "The public key is not published on %1.")
+                   .arg(host));
+          return;
+        }
+        Tell(Severity::kWarning, title,
              QCoreApplication::translate(
-                 "GTrC", "%1 did not return this public key.\n%2")
+                 "GTrC", "Could not ask %1 about this key.\n\n%2")
                  .arg(host, error));
       });
   return gf::cmd::Outcome<gf::cmd::Unit>::Success({});
diff --git a/src/m_key_server_sync/VKSInterface.cpp b/src/m_key_server_sync/VKSInterface.cpp
index 006729c..5bceb6a 100644
--- a/src/m_key_server_sync/VKSInterface.cpp
+++ b/src/m_key_server_sync/VKSInterface.cpp
@@ -41,6 +41,7 @@
 #include <QUrlQuery>
 
 #include "GFModule.h"
+#include "KeyServerBatchLogic.h"
 
 namespace {
 /// Matches the HKP side; without it a stalled server hangs the caller forever.
@@ -144,7 +145,12 @@ void VKSInterface::RequestVerify(const QString& token,
 
 void VKSInterface::on_reply_finished(QNetworkReply* reply) {
   if (reply->error() != QNetworkReply::NoError) {
-    emit SignalErrorOccurred(reply->errorString(), reply->readAll());
+    // A 404 from a lookup is an answer, not a failure: the server has no
+    // such key. Said plainly, rather than as Qt's transfer error.
+    const auto error = reply->error() == QNetworkReply::ContentNotFoundError
+                           ? KeyServerBatchLogic::NotFoundText()
+                           : reply->errorString();
+    emit SignalErrorOccurred(error, reply->readAll());
     reply->deleteLater();
     return;
   }
diff --git a/src/m_key_server_sync/test/KeyServerBatchLogicTest.cpp b/src/m_key_server_sync/test/KeyServerBatchLogicTest.cpp
index 2404b14..9359410 100644
--- a/src/m_key_server_sync/test/KeyServerBatchLogicTest.cpp
+++ b/src/m_key_server_sync/test/KeyServerBatchLogicTest.cpp
@@ -71,10 +71,12 @@ TEST(KeyServerBatchLogicTest, AKeyWithoutAFingerprintIsDropped) {
 }
 
 TEST(KeyServerBatchLogicTest, BlocksJoinIntoOneImportOnSeparateLines) {
-  const QByteArray a = "-----BEGIN PGP PUBLIC KEY BLOCK-----\nA\n"
-                       "-----END PGP PUBLIC KEY BLOCK-----";
-  const QByteArray b = "-----BEGIN PGP PUBLIC KEY BLOCK-----\nB\n"
-                       "-----END PGP PUBLIC KEY BLOCK-----\n";
+  const QByteArray a =
+      "-----BEGIN PGP PUBLIC KEY BLOCK-----\nA\n"
+      "-----END PGP PUBLIC KEY BLOCK-----";
+  const QByteArray b =
+      "-----BEGIN PGP PUBLIC KEY BLOCK-----\nB\n"
+      "-----END PGP PUBLIC KEY BLOCK-----\n";
   const auto joined = KeyServerBatchLogic::JoinForImport({a, "", "  \n", b});
   EXPECT_EQ(joined, a + "\n" + b);
   EXPECT_TRUE(KeyServerBatchLogic::JoinForImport({}).isEmpty());
@@ -90,8 +92,19 @@ TEST(KeyServerBatchLogicTest, SummariesCountAndListFailures) {
   EXPECT_TRUE(partial.contains("1 of 2"));
   EXPECT_TRUE(partial.contains("BBBB: not found"));
 
-  const auto published = KeyServerBatchLogic::PublishSummary(
-      2, 2, "keys.openpgp.org", {});
+  const auto published =
+      KeyServerBatchLogic::PublishSummary(2, 2, "keys.openpgp.org", {});
   EXPECT_TRUE(published.contains("2 of 2"));
   EXPECT_TRUE(published.contains("keys.openpgp.org"));
 }
+
+// "The server does not have this key" is an answer, told apart from a real
+// failure by the caller, and never shown in the transport's own wording.
+TEST(KeyServerBatchLogicTest, NotFoundIsItsOwnPlainAnswer) {
+  EXPECT_TRUE(
+      KeyServerBatchLogic::IsNotFound(KeyServerBatchLogic::NotFoundText()));
+  EXPECT_FALSE(KeyServerBatchLogic::IsNotFound(
+      "Error transferring https://keys.openpgp.org/vks/v1/by-fingerprint/X - "
+      "server replied: "));
+  EXPECT_FALSE(KeyServerBatchLogic::NotFoundText().contains("http"));
+}
diff --git a/src/m_key_server_sync/ui/main.lua b/src/m_key_server_sync/ui/main.lua
index 5c9c1e9..ef0d17a 100644
--- a/src/m_key_server_sync/ui/main.lua
+++ b/src/m_key_server_sync/ui/main.lua
@@ -37,15 +37,32 @@ ui.action { id = "refresh", anchor = ui.anchor("key.details.actions"),
 ui.action { id = "check", anchor = ui.anchor("key.details.actions"),
             command = check, update = for_this_key }
 
--- The same operations on whatever a key list has selected: one key or many,
--- sent as one batch.
-local function for_the_selection(ctx)
-  local keys = ctx.keys
-  if not keys or #keys == 0 then return { visible = false } end
-  return { args = { keys = keys } }
+-- The same operations on the keys a key list acts on: the checked ones, or
+-- else the selection, sent as one batch. A key group is not a key a server
+-- holds, so a target that includes one greys the entries out.
+local function for_the_targets(hide_when_empty)
+  return function(ctx)
+    local keys = ctx.keys
+    local none = not keys or #keys == 0
+    if none and not ctx:has_key_group() then
+      if hide_when_empty then return { visible = false } end
+      return { enabled = false }
+    end
+    if ctx:has_key_group() or none then return { enabled = false } end
+    return { args = { keys = keys } }
+  end
 end
 
+-- Right-clicking a key: shown only when there is something to act on.
 ui.action { id = "publish_selected", anchor = ui.anchor("key.list.context"),
-            command = publish, update = for_the_selection }
+            command = publish, update = for_the_targets(true) }
 ui.action { id = "refresh_selected", anchor = ui.anchor("key.list.context"),
-            command = refresh, update = for_the_selection }
+            command = refresh, update = for_the_targets(true) }
+
+-- Key Management's menu bar: always there, greyed until keys are checked.
+ui.action { id = "publish_checked",
+            anchor = ui.anchor("key.manager.menu.operations"),
+            command = publish, update = for_the_targets(false) }
+ui.action { id = "refresh_checked",
+            anchor = ui.anchor("key.manager.menu.operations"),
+            command = refresh, update = for_the_targets(false) }