From 8d7539fa0e1256aeb3d2db8a0a63633a1c7d25eb Mon Sep 17 00:00:00 2001 From: Patrick Sean Klein Date: Wed, 31 May 2023 19:15:22 +0100 Subject: [PATCH] Implement request builder. --- src/core/NetworkRequest.cpp | 121 +++++++++++++++++++++++++---------- src/core/NetworkRequest.h | 86 +++++++++++++++++-------- tests/TestNetworkRequest.cpp | 41 +++++++----- 3 files changed, 168 insertions(+), 80 deletions(-) diff --git a/src/core/NetworkRequest.cpp b/src/core/NetworkRequest.cpp index c16a9991e..d27288bca 100644 --- a/src/core/NetworkRequest.cpp +++ b/src/core/NetworkRequest.cpp @@ -6,6 +6,18 @@ #include #include +namespace { + QList> createDefaultHeaders() { + QList> headers; + headers.append(QPair{"User-Agent", "KeePassXC"}); + return headers; + } +} + +static constexpr int DEFAULT_MAX_REDIRECTS = 5; +static constexpr std::chrono::milliseconds DEFAULT_TIMEOUT = std::chrono::seconds(5); +static QList> DEFAULT_HEADERS = createDefaultHeaders(); + namespace { QUrl getRedirectTarget(QNetworkReply* reply) @@ -51,8 +63,6 @@ void NetworkRequest::fetch(const QUrl& url) connect(m_reply, &QNetworkReply::finished, this, &NetworkRequest::fetchFinished); connect(m_reply, &QIODevice::readyRead, this, &NetworkRequest::fetchReadyRead); - - m_timeout.start(); } void NetworkRequest::fetchFinished() @@ -81,12 +91,10 @@ void NetworkRequest::fetchFinished() redirectTarget = url.resolved(redirectTarget); } // Request the redirect target - qDebug() << "Following redirect to" << redirectTarget; m_redirects += 1; fetch(redirectTarget); return; } else { - qDebug() << "Too many redirects"; emit failure(); return; } @@ -142,32 +150,25 @@ QUrl NetworkRequest::URL() const return m_requested_url; } -void NetworkRequest::setMaxRedirects(int maxRedirects) -{ - m_maxRedirects = std::max(0, maxRedirects); -} - NetworkRequest::NetworkRequest( - QUrl targetURL, - int maxRedirects, - std::chrono::milliseconds timeoutDuration, - QList> headers, - QNetworkAccessManager* manager) - : m_reply(nullptr) + QUrl targetURL, bool allowInsecure, unsigned int maxRedirects, std::chrono::milliseconds timeoutDuration, QList> headers, + QNetworkAccessManager* manager) + : m_manager(manager) + , m_reply(nullptr) , m_finished(false) , m_maxRedirects(maxRedirects) , m_redirects(0) - , m_timeoutDuration(timeoutDuration) , m_headers(headers) , m_requested_url(targetURL) { - m_manager = manager ? manager : getNetMgr(); connect(&m_timeout, &QTimer::timeout, this, &NetworkRequest::fetchTimeout); - m_timeout.setInterval(m_timeoutDuration); + m_timeout.setInterval(timeoutDuration); m_timeout.setSingleShot(true); - fetch(m_requested_url); + if(!allowInsecure) { + // TODO + } } const QString& NetworkRequest::ContentType() const @@ -180,11 +181,6 @@ const QHash& NetworkRequest::ContentTypeParameters() const return m_content_type_parameters; } -void NetworkRequest::setTimeout(std::chrono::milliseconds timeoutDuration) -{ - m_timeoutDuration = timeoutDuration; -} - void NetworkRequest::fetchTimeout() { if(m_finished) @@ -200,16 +196,71 @@ QNetworkReply* NetworkRequest::Reply() const return m_reply; } -NetworkRequest createRequest(QUrl target,int maxRedirects, - std::chrono::milliseconds timeoutDuration, - QList> additionalHeaders, - QNetworkAccessManager* manager) +void NetworkRequest::fetch() { - // Append user agent unless given - if (std::none_of(additionalHeaders.begin(), additionalHeaders.end(), [](const auto& pair) { - return pair.first == "User-Agent"; - })) { - additionalHeaders.append(QPair{"User-Agent", "KeePassXC"}); - } - return NetworkRequest(std::move(target), maxRedirects, timeoutDuration, additionalHeaders, manager); + m_timeout.start(); + fetch(m_requested_url); +} + +NetworkRequestBuilder::NetworkRequestBuilder() +{ + this->setAllowInsecure(false); + this->setMaxRedirects(DEFAULT_MAX_REDIRECTS); + this->setTimeout(DEFAULT_TIMEOUT); + this->setHeaders(DEFAULT_HEADERS); + this->setManager(nullptr); +} + +NetworkRequestBuilder::NetworkRequestBuilder(QUrl url) : NetworkRequestBuilder() +{ + this->setUrl(url); +} + +NetworkRequestBuilder& NetworkRequestBuilder::setManager(QNetworkAccessManager* manager) +{ + m_manager = manager ? manager : getNetMgr(); + return *this; +} + +NetworkRequest NetworkRequestBuilder::build() +{ + return NetworkRequest(m_url, m_allowInsecure, m_maxRedirects, m_timeoutDuration, m_headers, m_manager); +} + +NetworkRequestBuilder& NetworkRequestBuilder::setHeaders(QList> headers) +{ + m_headers = headers; + + // Append user agent unless given + if (std::none_of(m_headers.begin(), m_headers.end(), [](const auto& pair) { + return pair.first == "User-Agent"; + })) { + m_headers.append(QPair{"User-Agent", "KeePassXC"}); + } + + return *this; +} + +NetworkRequestBuilder& NetworkRequestBuilder::setTimeout(std::chrono::milliseconds timeoutDuration) +{ + m_timeoutDuration = timeoutDuration; + return *this; +} + +NetworkRequestBuilder& NetworkRequestBuilder::setUrl(QUrl url) +{ + m_url = url; + return *this; +} + +NetworkRequestBuilder& NetworkRequestBuilder::setAllowInsecure(bool allowInsecure) +{ + m_allowInsecure = allowInsecure; + return *this; +} + +NetworkRequestBuilder& NetworkRequestBuilder::setMaxRedirects(unsigned int maxRedirects) +{ + m_maxRedirects = maxRedirects; + return *this; } diff --git a/src/core/NetworkRequest.h b/src/core/NetworkRequest.h index 1746add65..f5ce3302a 100644 --- a/src/core/NetworkRequest.h +++ b/src/core/NetworkRequest.h @@ -47,31 +47,19 @@ class NetworkRequest : public QObject // Request parameters QTimer m_timeout; - int m_maxRedirects; - int m_redirects; - std::chrono::milliseconds m_timeoutDuration; + unsigned int m_maxRedirects; + unsigned int m_redirects; QList> m_headers; QUrl m_requested_url; public: // TODO Disallow insecure connections by default? - explicit NetworkRequest(QUrl targetURL, int maxRedirects, + explicit NetworkRequest(QUrl targetURL, bool allowInsecure, unsigned int maxRedirects, std::chrono::milliseconds timeoutDuration, QList> headers, QNetworkAccessManager* manager = nullptr); - ~NetworkRequest() override; - /** - * Sets the maximum number of redirects to follow. - * @param maxRedirects The maximum number of redirects to follow. Must be >= 0. - */ - void setMaxRedirects(int maxRedirects); - - /** - * Sets the timeout duration for the request. This is the maximum time the request may take, including redirects. - * @param timeoutDuration The duration of the timeout in milliseconds. - */ - void setTimeout(std::chrono::milliseconds timeoutDuration); + void fetch(); void cancel(); @@ -96,6 +84,7 @@ public: */ const QHash& ContentTypeParameters() const; + ~NetworkRequest() override; private: void reset(); void fetch(const QUrl& url); @@ -109,17 +98,58 @@ signals: void failure(); }; -/** - * Creates a NetworkRequest with default parameters. - * @param maxRedirects - * @param timeoutDuration - * @param headers - * @param manager - * @return - */ -NetworkRequest createRequest(QUrl target, int maxRedirects = 5, - std::chrono::milliseconds timeoutDuration = std::chrono::milliseconds(5000), - QList> additionalHeaders = {}, - QNetworkAccessManager* manager = nullptr); +class NetworkRequestBuilder { + QUrl m_url; + bool m_allowInsecure; + unsigned int m_maxRedirects; + std::chrono::milliseconds m_timeoutDuration; + QList> m_headers; + QNetworkAccessManager* m_manager; + + NetworkRequestBuilder(); +public: + explicit NetworkRequestBuilder(QUrl url); + + /** + * Sets the URL to request. + * @param url The URL to request. + */ + NetworkRequestBuilder& setUrl(QUrl url); + + /** + * Sets whether insecure connections are allowed. + * @param allowInsecure + */ + NetworkRequestBuilder& setAllowInsecure(bool allowInsecure); + + /** + * Sets the maximum number of redirects to follow. + * @param maxRedirects The maximum number of redirects to follow. Must be >= 0. + */ + NetworkRequestBuilder& setMaxRedirects(unsigned int maxRedirects); + + /** + * Sets the timeout duration for the request. This is the maximum time the request may take, including redirects. + * @param timeoutDuration The duration of the timeout in milliseconds. + */ + NetworkRequestBuilder& setTimeout(std::chrono::milliseconds timeoutDuration); + + /** + * Sets the headers to send with the request. + * @param headers + */ + NetworkRequestBuilder& setHeaders(QList> headers); + + /** + * Sets the QNetworkAccessManager to use for the request. + * @param manager + */ + NetworkRequestBuilder& setManager(QNetworkAccessManager* manager); + + /** + * Builds a new NetworkRequest based on the configured parameters. + */ + NetworkRequest build(); +}; #endif // KEEPASSXC_NETWORKREQUEST_H diff --git a/tests/TestNetworkRequest.cpp b/tests/TestNetworkRequest.cpp index 026dcf492..7063772cb 100644 --- a/tests/TestNetworkRequest.cpp +++ b/tests/TestNetworkRequest.cpp @@ -11,6 +11,12 @@ using ContentTypeParameters_t = QHash; Q_DECLARE_METATYPE(ContentTypeParameters_t); Q_DECLARE_METATYPE(std::chrono::milliseconds); +namespace { + void processRequests() { + QTest::qWait(300); + } +} + void TestNetworkRequest::testNetworkRequest() { QFETCH(QUrl, requestedURL); @@ -39,7 +45,7 @@ void TestNetworkRequest::testNetworkRequest() } // Create request - NetworkRequest request = createRequest(requestedURL, 5, std::chrono::milliseconds{5000}, QList>{}, &manager); + NetworkRequest request = NetworkRequestBuilder(requestedURL).setManager(&manager).build(); QString actualContent; bool didError = false, didSucceed = false; @@ -54,8 +60,8 @@ void TestNetworkRequest::testNetworkRequest() QSignalSpy errorSpy(&request, &NetworkRequest::failure); connect(&request, &NetworkRequest::failure, [&didError]() { didError = true; }); - - QTest::qWait(3*100); + request.fetch(); + processRequests(); // Ensures that predicates match - i.e., the header was set correctly QCOMPARE(manager.matchedRequests().length(), 1); @@ -134,7 +140,7 @@ void TestNetworkRequest::testNetworkRequestTimeout() reply.withFinishDelayUntil(&timer, &QTimer::timeout); // Create request - NetworkRequest request = createRequest(requestedURL, 5, timeout, QList>{}, &manager); + NetworkRequest request = NetworkRequestBuilder(requestedURL).setManager(&manager).setTimeout(timeout).build(); // Start timer timer.start(); @@ -149,8 +155,8 @@ void TestNetworkRequest::testNetworkRequestTimeout() QSignalSpy errorSpy(&request, &NetworkRequest::failure); connect(&request, &NetworkRequest::failure, [&didError]() { didError = true; }); - - QTest::qWait(3*100); + request.fetch(); + processRequests(); QTEST_ASSERT(didError || didSucceed); @@ -168,8 +174,8 @@ void TestNetworkRequest::testNetworkRequestTimeout_data() QTest::addColumn("delay"); QTest::addColumn("timeout"); - QTest::newRow("timeout") << true << std::chrono::milliseconds{200} << std::chrono::milliseconds{100}; - QTest::newRow("no timeout") << false << std::chrono::milliseconds{100} << std::chrono::milliseconds{200}; + QTest::newRow("timeout") << true << std::chrono::milliseconds{100} << std::chrono::milliseconds{50}; + QTest::newRow("no timeout") << false << std::chrono::milliseconds{50} << std::chrono::milliseconds{100}; } void TestNetworkRequest::testNetworkRequestRedirects() @@ -209,7 +215,8 @@ void TestNetworkRequest::testNetworkRequestRedirects() reply->withBody(QString{"test-content"}.toUtf8()); // Create request - NetworkRequest request = createRequest(requestedURL, maxRedirects, std::chrono::milliseconds{5000}, QList>{}, &manager); + NetworkRequest request = NetworkRequestBuilder(requestedURL).setManager(&manager) + .setMaxRedirects(maxRedirects).build(); bool didSucceed = false, didError = false; // Check request @@ -221,8 +228,8 @@ void TestNetworkRequest::testNetworkRequestRedirects() QSignalSpy errorSpy(&request, &NetworkRequest::failure); connect(&request, &NetworkRequest::failure, [&didError]() { didError = true; }); - - QTest::qWait(3*100); + request.fetch(); + processRequests(); QTEST_ASSERT(didError || didSucceed); // Ensures that predicates match - i.e., the header was set correctly @@ -239,10 +246,10 @@ void TestNetworkRequest::testNetworkRequestRedirects_data() QTest::addColumn("numRedirects"); QTest::addColumn("maxRedirects"); - QTest::newRow("all good (0)") << 0 << 5; - QTest::newRow("all good (1)") << 1 << 5; - QTest::newRow("all good (2)") << 2 << 5; - QTest::newRow("no good (1, 0)") << 1 << 0; - QTest::newRow("no good (2, 1)") << 2 << 1; - QTest::newRow("no good (3, 2)") << 3 << 2; + QTest::newRow("fewer redirects than allowed (0)") << 0 << 5; + QTest::newRow("fewer redirects than allowed (1)") << 1 << 5; + QTest::newRow("fewer redirects than allowed (2)") << 2 << 5; + QTest::newRow("more redirects than allowed (1, 0)") << 1 << 0; + QTest::newRow("more redirects than allowed (2, 1)") << 2 << 1; + QTest::newRow("more redirects than allowed (3, 2)") << 3 << 2; } \ No newline at end of file