Merge lp:~mandel/ubuntu-download-manager/follow-redirects into lp:ubuntu-download-manager

[Manuel de la Peña]

Ensure that redirects are followed from udm unless we are in a loop. If the download gets in a redirect loop a network error is emitted.

Ensure that redirects are followed from udm unless we are in a loop. If the download gets in a redirect loop a network error is emitted.

[lp:~mandel/ubuntu-download-manager/follow-redirects]

228. By Manuel de la Peña on 2014-02-04

Raise an error if we have file system issues.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:227
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/322/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/211
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/211

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-download-manager-ci/322/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:228
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/323/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/212
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/212

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-download-manager-ci/323/rebuild

review: Approve (continuous-integration)

[Roberto Alsina (ralsina)]

review: Approve

[lp:~mandel/ubuntu-download-manager/follow-redirects]

229. By Manuel de la Peña on 2014-02-04

Fix some small space alignment issue in the code.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:229
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/324/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/213
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/213

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-download-manager-ci/324/rebuild

review: Approve (continuous-integration)

[Mike McCracken (mikemc)]

review: Approve

=== modified file 'ubuntu-download-manager-priv/downloads/file_download.cpp'
--- ubuntu-download-manager-priv/downloads/file_download.cpp	2014-02-03 15:30:58 +0000
+++ ubuntu-download-manager-priv/downloads/file_download.cpp	2014-02-04 16:23:22 +0000
@@ -291,16 +291,54 @@
 }
 
 void
-FileDownload::onFinished() {
+FileDownload::onRedirect(QUrl redirect) {
+    LOG(INFO) << "Following redirect to" << redirect;
+    // update the _url value and perform a second request to try and get the data
+    if (_visitedUrls.contains(redirect)) {
+        // we are in a loop!!! we have to raise an error about this.
+        LOG(WARNING) << "Redirect loop found";
+        NetworkErrorStruct err(QNetworkReply::ContentNotFoundError);
+        emit networkError(err);
+        emitError(NETWORK_ERROR);
+        return;
+    }
+    _visitedUrls.append(_url);
+    _url = redirect;
+
+    // clean the reply
+    disconnectFromReplySignals();
+    _reply->deleteLater();
+    _reply = nullptr;
+
+    // clean the local file
+    cleanUpCurrentData();
+
+    // perform again the request but do not emit started signal
+    _currentData = FileManager::instance()->createFile(_filePath);
+    bool canWrite = _currentData->open(QIODevice::ReadWrite | QFile::Append);
+
+    if (!canWrite) {
+        emitError(FILE_SYSTEM_ERROR);
+        return;
+    }
+
+    _reply = _requestFactory->get(buildRequest());
+    _reply->setReadBufferSize(throttle());
+
+    connectToReplySignals();
+}
+
+void
+FileDownload::onDownloadCompleted() {
     TRACE << _url;
 
     // if the hash is present we check it
     if (!_hash.isEmpty()) {
         emit processing(filePath());
         _currentData->reset();
-	QCryptographicHash hash(_algo);
-	// addData is smart enough to not load the entire file in memory
-	hash.addData(_currentData->device());
+        QCryptographicHash hash(_algo);
+        // addData is smart enough to not load the entire file in memory
+        hash.addData(_currentData->device());
         QString fileSig = QString(hash.result().toHex());
 
         if (fileSig != _hash) {
@@ -316,7 +354,8 @@
     // finish signals once the command was done (or an error ocurred in
     // the command execution.
     if (metadata().contains(METADATA_COMMAND_KEY)) {
-        // just emit processing if we DO NOT have a hash because else we already emitted it.
+        // just emit processing if we DO NOT have a hash because else we
+        // already emitted it.
         if (_hash.isEmpty()) {
             emit processing(filePath());
         }
@@ -368,6 +407,21 @@
 }
 
 void
+FileDownload::onFinished() {
+    TRACE << _url;
+    // the reply has finished but the resource might have been moved
+    // and we must do a new request
+    auto redirect = _reply->attribute(
+        QNetworkRequest::RedirectionTargetAttribute).toUrl();
+
+    if(!redirect.isEmpty() && redirect != _url) {
+        onRedirect(redirect);
+    } else {
+        onDownloadCompleted();
+    }
+}
+
+void
 FileDownload::onSslErrors(const QList<QSslError>& errors) {
     TRACE << errors;
     if (!_reply->canIgnoreSslErrors(errors)) {
@@ -539,7 +593,7 @@
     if (!secondPart.isEmpty()) {
         secondPart = "." + secondPart;
         firstPart = path.left(path.size() - secondPart.size());
-    } 
+    }
 
     // Try with an ever-increasing number suffix, until we've reached a file
     // that does not yet exist.
@@ -557,7 +611,7 @@
 
 void
 FileDownload::cleanUpCurrentData() {
-    bool success = true; 
+    bool success = true;
     QFile::FileError error = QFile::NoError;
     if (_currentData != nullptr) {
         success = _currentData->remove();

=== modified file 'ubuntu-download-manager-priv/downloads/file_download.h'
--- ubuntu-download-manager-priv/downloads/file_download.h	2014-02-03 13:34:10 +0000
+++ ubuntu-download-manager-priv/downloads/file_download.h	2014-02-04 16:23:22 +0000
@@ -112,6 +112,8 @@
     void init();
     void onDownloadProgress(qint64 currentProgress, qint64);
     void onError(QNetworkReply::NetworkError);
+    void onRedirect(QUrl redirect);
+    void onDownloadCompleted();
     void onFinished();
     void onSslErrors(const QList<QSslError>&);
     void onProcessError(QProcess::ProcessError error);
@@ -131,6 +133,7 @@
     QCryptographicHash::Algorithm _algo;
     NetworkReply* _reply = nullptr;
     File* _currentData = nullptr;
+    QList<QUrl> _visitedUrls;
 };
 
 }  // Daemon

=== modified file 'ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.cpp'
--- ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.cpp	2014-01-27 16:36:44 +0000
+++ ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.cpp	2014-02-04 16:23:22 +0000
@@ -153,6 +153,12 @@
 }
 
 void
+FakeNetworkReply::setAttribute(QNetworkRequest::Attribute code,
+                               QUrl url) {
+    _attrs[code] = url;
+}
+
+void
 FakeNetworkReply::clearAttribute(QNetworkRequest::Attribute code) {
     if (_attrs.contains(code)) {
         _attrs.remove(code);

=== modified file 'ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.h'
--- ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.h	2014-01-27 16:36:44 +0000
+++ ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.h	2014-02-04 16:23:22 +0000
@@ -65,6 +65,8 @@
                       int value);
     void setAttribute(QNetworkRequest::Attribute code,
                       QString message);
+    void setAttribute(QNetworkRequest::Attribute code,
+                      QUrl url);
     void clearAttribute(QNetworkRequest::Attribute code);
 
  private:

=== modified file 'ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.cpp'
--- ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.cpp	2014-01-24 11:54:24 +0000
+++ ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.cpp	2014-02-04 16:23:22 +0000
@@ -32,6 +32,7 @@
 FakeRequestFactory::FakeRequestFactory(QObject *parent)
     : RequestFactory(parent),
       Fake() {
+    qRegisterMetaType<NetworkReply*>("NetworkReply*");
 }
 
 NetworkReply*
@@ -53,6 +54,7 @@
         // if we are recording we do set the recording of the returned reply
         reply->record();
     }
+    emit requestCreated(reply);
     return reply;
 }
 

=== modified file 'ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.h'
--- ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.h	2014-01-10 16:38:56 +0000
+++ ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.h	2014-02-04 16:23:22 +0000
@@ -50,6 +50,9 @@
     NetworkReply* get(const QNetworkRequest& request);
     QList<QSslCertificate> acceptedCertificates() override;
     void setAcceptedCertificates(const QList<QSslCertificate>& certs) override;
+
+ signals:
+    void requestCreated(NetworkReply*);
 };
 
 #endif  // FAKE_QNETWORK_ACCESS_MANAGER_H

=== modified file 'ubuntu-download-manager-tests/downloads/test_download.cpp'
--- ubuntu-download-manager-tests/downloads/test_download.cpp	2014-02-03 13:34:10 +0000
+++ ubuntu-download-manager-tests/downloads/test_download.cpp	2014-02-04 16:23:22 +0000
@@ -1986,3 +1986,178 @@
         QString("FILE SYSTEM ERROR: %1").arg(QFile::WriteError));
     _fileManager->setFile(nullptr);
 }
+
+void
+TestDownload::testRedirectCycle() {
+    // the test works as follows, create a request, pause and set the redirect
+    // header, wait until the second request is created via the signal from the
+    // request factory and set the redirect to a first url
+    QUrl redirectUrl("http://redirect.example.com");
+    _reqFactory->record();
+    QScopedPointer<FileDownload> download(new FileDownload(_id, _path, _isConfined,
+        _rootPath, _url, _metadata, _headers));
+
+    download->start();  // change state
+    download->startDownload();
+
+    // we need to set the data before we pause!!!
+    QList<MethodData> calledMethods = _reqFactory->calledMethods();
+    QCOMPARE(1, calledMethods.count());
+    FakeNetworkReply* reply = qobject_cast<FakeNetworkReply*>(
+        calledMethods[0].params().outParams()[0]);
+
+    // set the attr for a redirect to a new url
+    // makes the process to be executed
+    reply->setAttribute(QNetworkRequest::RedirectionTargetAttribute,
+        redirectUrl);
+    QSignalSpy replySpy(_reqFactory, SIGNAL(requestCreated(NetworkReply*)));
+    
+    // use a spy to wait for the second reply
+    reply->emitFinished();
+
+    QTRY_COMPARE(1, replySpy.count());
+
+    reply = qobject_cast<FakeNetworkReply*>(
+        replySpy.takeFirst().at(0).value<NetworkReply*>());
+
+    download->pause();
+    
+    // set the attr to be a loop
+    reply->setAttribute(QNetworkRequest::RedirectionTargetAttribute,
+        _url);
+
+    // emit finished and an errors should be emitted
+    QSignalSpy errorSpy(download.data(), SIGNAL(error(QString)));
+    QSignalSpy networkErrorSpy(download.data(),
+        SIGNAL(networkError(NetworkErrorStruct)));
+
+    reply->emitFinished();
+
+    QTRY_COMPARE(networkErrorSpy.count(), 1);
+    QTRY_COMPARE(errorSpy.count(), 1);
+}
+
+void
+TestDownload::testSingleRedirect() {
+    // ensure that a single redirect is followed
+    QUrl redirectUrl("http://redirect.example.com");
+    _reqFactory->record();
+    QScopedPointer<FileDownload> download(new FileDownload(_id, _path, _isConfined,
+        _rootPath, _url, _metadata, _headers));
+
+    download->start();  // change state
+    download->startDownload();
+
+    // we need to set the data before we pause!!!
+    QList<MethodData> calledMethods = _reqFactory->calledMethods();
+    QCOMPARE(1, calledMethods.count());
+    FakeNetworkReply* reply = qobject_cast<FakeNetworkReply*>(
+        calledMethods[0].params().outParams()[0]);
+
+    // set the attr for a redirect to a new url
+    // makes the process to be executed
+    reply->setAttribute(QNetworkRequest::RedirectionTargetAttribute,
+        redirectUrl);
+    QSignalSpy replySpy(_reqFactory, SIGNAL(requestCreated(NetworkReply*)));
+    
+    // use a spy to wait for the second reply
+    reply->emitFinished();
+
+    // ensure that a second request is performed
+    QTRY_COMPARE(1, replySpy.count());
+
+    reply = qobject_cast<FakeNetworkReply*>(
+        replySpy.takeFirst().at(0).value<NetworkReply*>());
+
+    download->pause();
+    
+    QSignalSpy errorSpy(download.data(), SIGNAL(error(QString)));
+    QSignalSpy networkErrorSpy(download.data(),
+        SIGNAL(networkError(NetworkErrorStruct)));
+    QSignalSpy finishedSpy(download.data(), SIGNAL(finished(QString)));
+
+    reply->emitFinished();
+
+    QTRY_COMPARE(networkErrorSpy.count(), 0);
+    QTRY_COMPARE(errorSpy.count(), 0);
+    QTRY_COMPARE(finishedSpy.count(), 1);
+}
+
+void
+TestDownload::testSeveralRedirects_data() {
+    QTest::addColumn<QStringList>("urls");
+
+    QStringList first;
+    first.append("http://first.com/resource");
+    first.append("http://first.com/moved_resource");
+    first.append("http://first.com/last");
+    QTest::newRow("Three redirects") << first;
+
+    QStringList second;
+    second.append("http://second.com/resource");
+    second.append("http://second.com/moved1");
+    second.append("http://second.com/moved2");
+    second.append("http://second.com/moved3");
+    second.append("http://second.com/moved4");
+    QTest::newRow("Five redirects") << second;
+
+    QStringList third;
+    third.append("http://third.com/resource");
+    third.append("http://third.com/moved1");
+    third.append("http://third.com/moved2");
+    third.append("http://third.com/moved3");
+    third.append("http://third.com/moved4");
+    third.append("http://third.com/moved5");
+    third.append("http://third.com/moved6");
+    QTest::newRow("Seven redirects") << third;
+
+}
+
+void
+TestDownload::testSeveralRedirects() {
+    QFETCH(QStringList, urls);
+    auto redirectCount = 0;
+    _reqFactory->record();
+    QScopedPointer<FileDownload> download(new FileDownload(_id, _path, _isConfined,
+        _rootPath, _url, _metadata, _headers));
+
+    download->start();  // change state
+    download->startDownload();
+
+    // we need to set the data before we pause!!!
+    QList<MethodData> calledMethods = _reqFactory->calledMethods();
+    QCOMPARE(1, calledMethods.count());
+    FakeNetworkReply* reply = qobject_cast<FakeNetworkReply*>(
+        calledMethods[0].params().outParams()[0]);
+
+    // redirect all the required times
+    foreach(const QString& url, urls) {
+        reply->setAttribute(QNetworkRequest::RedirectionTargetAttribute,
+            url);
+        QSignalSpy replySpy(_reqFactory, SIGNAL(requestCreated(NetworkReply*)));
+        
+        // use a spy to wait for the second reply
+        reply->emitFinished();
+
+        // ensure that a second request is performed
+        QTRY_COMPARE(1, replySpy.count());
+
+        reply = qobject_cast<FakeNetworkReply*>(
+            replySpy.takeFirst().at(0).value<NetworkReply*>());
+        redirectCount++;
+    }
+    
+    download->pause();
+    
+    QSignalSpy errorSpy(download.data(), SIGNAL(error(QString)));
+    QSignalSpy networkErrorSpy(download.data(),
+        SIGNAL(networkError(NetworkErrorStruct)));
+    QSignalSpy finishedSpy(download.data(), SIGNAL(finished(QString)));
+
+    reply->emitFinished();
+
+    QTRY_COMPARE(networkErrorSpy.count(), 0);
+    QTRY_COMPARE(errorSpy.count(), 0);
+    QTRY_COMPARE(finishedSpy.count(), 1);
+    QCOMPARE(redirectCount, urls.count());
+}

=== modified file 'ubuntu-download-manager-tests/downloads/test_download.h'
--- ubuntu-download-manager-tests/downloads/test_download.h	2014-01-27 16:36:44 +0000
+++ ubuntu-download-manager-tests/downloads/test_download.h	2014-02-04 16:23:22 +0000
@@ -154,6 +154,12 @@
     void testFileSystemErrorProgress();
     void testFileSystemErrorPause();
 
+    // test different redirects
+    void testRedirectCycle();
+    void testSingleRedirect();
+    void testSeveralRedirects_data();
+    void testSeveralRedirects();
+
  private:
     QString _id;
     bool _isConfined;