Significant updates to original PR

* Selecting one or more entries to download icons always forces the download (ie, if a new URL exists the new icon will be downloaded and set)
* Instead of downloading for each entry, the web url's are scraped from the provided entries and only those urls are downloaded. The icon is set for all entries that share a URL. This is useful if a group contains many entries that point to the same url, only 1 download call will occur.
* The icon download dialog displays whether you are doing one entry, many entries, or an entire group. It is also modal so you have to dismiss it to use KeePassXC again.
* The timeout setting is always enabled.
* Moved DuckDuckGo fallback notice into the download dialog.

src/CMakeLists.txt

@@ -27,6 +27,9 @@ set(keepassx_SOURCES
         core/Alloc.cpp
         core/AutoTypeAssociations.cpp
         core/AutoTypeMatch.cpp
+        core/Base32.cpp
+        core/Bootstrap.cpp
+        core/Clock.cpp
         core/Compare.cpp
         core/Config.cpp
         core/CsvParser.cpp
@@ -39,7 +42,6 @@ set(keepassx_SOURCES
         core/EntrySearcher.cpp
         core/FilePath.cpp
         core/FileWatcher.cpp
-        core/Bootstrap.cpp
         core/Group.cpp
         core/HibpOffline.cpp
         core/InactivityTimer.cpp
@@ -52,10 +54,8 @@ set(keepassx_SOURCES
         core/ScreenLockListenerPrivate.cpp
         core/TimeDelta.cpp
         core/TimeInfo.cpp
-        core/Clock.cpp
         core/Tools.cpp
         core/Translator.cpp
-        core/Base32.cpp
         cli/Utils.cpp
         cli/TextStream.cpp
         crypto/Crypto.cpp
@@ -102,8 +102,6 @@ set(keepassx_SOURCES
         gui/EditWidgetProperties.cpp
         gui/FileDialog.cpp
         gui/Font.cpp
-        gui/IconDownloader.cpp
-        gui/IconDownloaderDialog.cpp
         gui/IconModels.cpp
         gui/KeePass1OpenWidget.cpp
         gui/KMessageWidget.cpp
@@ -265,7 +263,12 @@ else()
 endif()
 
 if(WITH_XC_NETWORKING)
-    list(APPEND keepassx_SOURCES updatecheck/UpdateChecker.cpp gui/UpdateCheckDialog.cpp)
+    list(APPEND keepassx_SOURCES
+            core/IconDownloader.cpp
+            core/NetworkManager.cpp
+            gui/UpdateCheckDialog.cpp
+            gui/IconDownloaderDialog.cpp
+            updatecheck/UpdateChecker.cpp)
 endif()
 
 if(WITH_XC_TOUCHID)

src/cli/Analyze.cpp

@@ -50,7 +50,8 @@ int Analyze::executeWithDatabase(QSharedPointer<Database> database, QSharedPoint
     QString hibpDatabase = parser->value(Analyze::HIBPDatabaseOption);
     QFile hibpFile(hibpDatabase);
     if (!hibpFile.open(QFile::ReadOnly)) {
-        errorTextStream << QObject::tr("Failed to open HIBP file %1: %2").arg(hibpDatabase).arg(hibpFile.errorString()) << endl;
+        errorTextStream << QObject::tr("Failed to open HIBP file %1: %2").arg(hibpDatabase).arg(hibpFile.errorString())
+                        << endl;
         return EXIT_FAILURE;
     }
 

src/core/Config.cpp

@@ -185,6 +185,7 @@ void Config::init(const QString& fileName)
     m_defaults.insert("AutoTypeStartDelay", 500);
     m_defaults.insert("UseGroupIconOnEntryCreation", true);
     m_defaults.insert("IgnoreGroupExpansion", true);
+    m_defaults.insert("FaviconDownloadTimeout", 10);
     m_defaults.insert("security/clearclipboard", true);
     m_defaults.insert("security/clearclipboardtimeout", 10);
     m_defaults.insert("security/lockdatabaseidle", false);

src/gui/IconDownloader.cpp → src/core/IconDownloader.cpp

@@ -17,27 +17,27 @@
 
 #include "IconDownloader.h"
 #include "core/Config.h"
+#include "core/NetworkManager.h"
 
-#ifdef WITH_XC_NETWORKING
 #include <QHostInfo>
-#include <QNetworkAccessManager>
 #include <QtNetwork>
-#endif
+
+#define MAX_REDIRECTS 5
 
 IconDownloader::IconDownloader(QObject* parent)
     : QObject(parent)
-#ifdef WITH_XC_NETWORKING
-    , m_netMgr(new QNetworkAccessManager(this))
     , m_reply(nullptr)
-#endif
+    , m_redirects(0)
 {
+    m_timeout.setSingleShot(true);
+    connect(&m_timeout, SIGNAL(timeout()), SLOT(abortDownload()));
 }
 
 IconDownloader::~IconDownloader()
 {
+    abortDownload();
 }
 
-#ifdef WITH_XC_NETWORKING
 namespace
 {
     // Try to get the 2nd level domain of the host part of a QUrl. For example,
@@ -55,8 +55,9 @@ namespace
     QUrl convertVariantToUrl(const QVariant& var)
     {
         QUrl url;
-        if (var.canConvert<QUrl>())
+        if (var.canConvert<QUrl>()) {
             url = var.toUrl();
+        }
         return url;
     }
 
@@ -67,21 +68,23 @@ namespace
         return url;
     }
 } // namespace
-#endif
 
-void IconDownloader::downloadFavicon(Entry* entry)
+void IconDownloader::downloadFavicon(const QString& entryUrl)
 {
-#ifdef WITH_XC_NETWORKING
-    m_entry = entry;
-    m_url = entry->url();
+    m_url = entryUrl;
+    QUrl url(m_url);
+    if (!url.isValid()) {
+        return;
+    }
+
     m_redirects = 0;
     m_urlsToTry.clear();
 
-    if (m_url.scheme().isEmpty()) {
-        m_url.setUrl(QString("https://%1").arg(m_url.toString()));
+    if (url.scheme().isEmpty()) {
+        url.setUrl(QString("https://%1").arg(url.toString()));
     }
 
-    QString fullyQualifiedDomain = m_url.host();
+    QString fullyQualifiedDomain = url.host();
 
     // Determine if host portion of URL is an IP address by resolving it and
     // searching for a match with the returned address(es).
@@ -113,38 +116,51 @@ void IconDownloader::downloadFavicon(Entry* entry)
     }
 
     // Add a direct pull of the website's own favicon.ico file
-    m_urlsToTry.append(QUrl(m_url.scheme() + "://" + fullyQualifiedDomain + "/favicon.ico"));
+    m_urlsToTry.append(QUrl(url.scheme() + "://" + fullyQualifiedDomain + "/favicon.ico"));
 
     // Also try a direct pull of the second-level domain (if possible)
     if (!hostIsIp && fullyQualifiedDomain != secondLevelDomain) {
-        m_urlsToTry.append(QUrl(m_url.scheme() + "://" + secondLevelDomain + "/favicon.ico"));
+        m_urlsToTry.append(QUrl(url.scheme() + "://" + secondLevelDomain + "/favicon.ico"));
     }
 
+    int timeout = config()->get("FaviconDownloadTimeout", 10).toInt();
+    m_timeout.start(timeout * 1000);
+
     // Use the first URL to start the download process
     // If a favicon is not found, the next URL will be tried
-    startFetchFavicon(m_urlsToTry.takeFirst());
-#else
-    Q_UNUSED(entry);
-#endif
+    fetchFavicon(m_urlsToTry.takeFirst());
+}
+
+void IconDownloader::abortDownload()
+{
+    if (m_reply) {
+        m_reply->abort();
+    }
+}
+
+void IconDownloader::fetchFavicon(const QUrl& url)
+{
+    m_bytesReceived.clear();
+    m_fetchUrl = url;
+
+    QNetworkRequest request(url);
+    m_reply = getNetMgr()->get(request);
+
+    connect(m_reply, &QNetworkReply::finished, this, &IconDownloader::fetchFinished);
+    connect(m_reply, &QIODevice::readyRead, this, &IconDownloader::fetchReadyRead);
 }
 
 void IconDownloader::fetchReadyRead()
 {
-#ifdef WITH_XC_NETWORKING
     m_bytesReceived += m_reply->readAll();
-#endif
 }
 
 void IconDownloader::fetchFinished()
 {
-#ifdef WITH_XC_NETWORKING
     QImage image;
-    bool fallbackEnabled = config()->get("security/IconDownloadFallback", false).toBool();
+    QString url = m_url;
+
     bool error = (m_reply->error() != QNetworkReply::NoError);
-    if (m_reply->error() == QNetworkReply::HostNotFoundError || m_reply->error() == QNetworkReply::TimeoutError) {
-        emit iconReceived(image, m_entry);
-        return;
-    }
     QUrl redirectTarget = getRedirectTarget(m_reply);
 
     m_reply->deleteLater();
@@ -154,12 +170,12 @@ void IconDownloader::fetchFinished()
         if (redirectTarget.isValid()) {
             // Redirected, we need to follow it, or fall through if we have
             // done too many redirects already.
-            if (m_redirects < 5) {
+            if (m_redirects < MAX_REDIRECTS) {
                 m_redirects++;
-                if (redirectTarget.isRelative())
+                if (redirectTarget.isRelative()) {
                     redirectTarget = m_fetchUrl.resolved(redirectTarget);
-                startFetchFavicon(redirectTarget);
-                return;
+                }
+                m_urlsToTry.prepend(redirectTarget);
             }
         } else {
             // No redirect, and we theoretically have some icon data now.
@@ -168,44 +184,14 @@ void IconDownloader::fetchFinished()
     }
 
     if (!image.isNull()) {
-        emit iconReceived(image, m_entry);
-        return;
+        // Valid icon received
+        emit finished(url, image);
     } else if (!m_urlsToTry.empty()) {
+        // Try the next url
         m_redirects = 0;
-        startFetchFavicon(m_urlsToTry.takeFirst());
-        return;
+        fetchFavicon(m_urlsToTry.takeFirst());
     } else {
-        if (!fallbackEnabled) {
-            emit fallbackNotEnabled();
-        } else {
-            emit iconError(m_entry);
-        }
-    }
-    emit iconReceived(image, m_entry);
-#endif
-}
-
-void IconDownloader::abortRequest()
-{
-#ifdef WITH_XC_NETWORKING
-    if (m_reply) {
-        m_reply->abort();
+        // No icon found
+        emit finished(url, image);
     }
-#endif
-}
-
-void IconDownloader::startFetchFavicon(const QUrl& url)
-{
-#ifdef WITH_XC_NETWORKING
-    m_bytesReceived.clear();
-    m_fetchUrl = url;
-
-    QNetworkRequest request(url);
-    m_reply = m_netMgr->get(request);
-
-    connect(m_reply, &QNetworkReply::finished, this, &IconDownloader::fetchFinished);
-    connect(m_reply, &QIODevice::readyRead, this, &IconDownloader::fetchReadyRead);
-#else
-    Q_UNUSED(url);
-#endif
 }

src/gui/IconDownloader.h → src/core/IconDownloader.h

@@ -15,56 +15,48 @@
  *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef KEEPASSX_ICONDOWNLOADER_H
-#define KEEPASSX_ICONDOWNLOADER_H
+#ifndef KEEPASSXC_ICONDOWNLOADER_H
+#define KEEPASSXC_ICONDOWNLOADER_H
 
+#include <QImage>
 #include <QObject>
+#include <QTimer>
 #include <QUrl>
-#include <QImage>
 
-#include "config-keepassx.h"
-#include "core/Entry.h"
 #include "core/Global.h"
 
-#ifdef WITH_XC_NETWORKING
-class QNetworkAccessManager;
 class QNetworkReply;
-#endif
 
 class IconDownloader : public QObject
 {
     Q_OBJECT
 
 public:
     explicit IconDownloader(QObject* parent = nullptr);
-    ~IconDownloader();
+    ~IconDownloader() override;
 
-    void downloadFavicon(Entry* entry);
-
-public slots:
-    void startFetchFavicon(const QUrl& url);
-    void abortRequest();
+    void downloadFavicon(const QString& entryUrl);
 
 signals:
-    void iconReceived(const QImage&, Entry*);
-    void iconError(Entry*);
-    void fallbackNotEnabled();
+    void finished(const QString& entryUrl, const QImage& image);
+
+public slots:
+    void abortDownload();
 
 private slots:
     void fetchFinished();
     void fetchReadyRead();
 
 private:
-#ifdef WITH_XC_NETWORKING
-    QUrl m_url;
-    Entry* m_entry;
+    void fetchFavicon(const QUrl& url);
+
+    QString m_url;
     QUrl m_fetchUrl;
     QList<QUrl> m_urlsToTry;
     QByteArray m_bytesReceived;
-    QNetworkAccessManager* m_netMgr;
     QNetworkReply* m_reply;
+    QTimer m_timeout;
     int m_redirects;
-#endif
 };
 
-#endif // KEEPASSX_ICONDOWNLOADER_H
+#endif // KEEPASSXC_ICONDOWNLOADER_H

src/core/NetworkManager.cpp

@@ -0,0 +1,33 @@
+/*
+ *  Copyright (C) 2019 KeePassXC Team <[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
+ *  the Free Software Foundation, either version 2 or (at your option)
+ *  version 3 of the License.
+ *
+ *  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 "config-keepassx.h"
+
+#ifdef WITH_XC_NETWORKING
+#include "NetworkManager.h"
+
+#include <QCoreApplication>
+
+QNetworkAccessManager* g_netMgr = nullptr;
+QNetworkAccessManager* getNetMgr()
+{
+    if (!g_netMgr) {
+        g_netMgr = new QNetworkAccessManager(QCoreApplication::instance());
+    }
+    return g_netMgr;
+}
+#endif