Merge lp:~mandel/ubuntu-download-manager/allow-client-object-path into lp:ubuntu-download-manager

[Manuel de la Peña]

Added the new metadata key 'objectpath' where the client can propose a uuid for the download that will be used to create the path.

Added the new metadata key 'objectpath' where the client can propose a uuid for the download that will be used to create the path. The string text, which must be formatted as five hex fields separated by '-', e.g., "{xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx}" where 'x' is a hex digit. The curly braces shown here are optional, but it is normal to include them. If the conversion fails the download manager will generate a correct uuid.

[Manuel de la Peña (mandel)]

I have added documentation to the following wiki page: https://wiki.ubuntu.com/DownloadService it contains some example code in python and I'll add more related to the group download.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:126
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/41/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-amd64-ci/41
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-armhf-ci/32

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

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

Looks good. I'll check about using UUIDs in the client, and will open a bug if needed.

review: Approve

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

=== modified file 'libubuntudownloadmanager/apparmor.cpp'
--- libubuntudownloadmanager/apparmor.cpp	2013-09-10 15:36:23 +0000
+++ libubuntudownloadmanager/apparmor.cpp	2013-09-10 15:36:23 +0000
@@ -41,9 +41,8 @@
         _uuidFactory = new UuidFactory();
     }
 
-    QPair<QUuid, QString> getUuidString(QString path) {
+    QPair<QUuid, QString> getUuidString(QUuid id, QString path) {
         qDebug() << __PRETTY_FUNCTION__ << path;
-        QUuid id = _uuidFactory->createUuid();
         QString idString = path + "/" + id.toString().replace(
             QRegExp("[-{}]"), "");
         qDebug() << "Download path is" << idString;
@@ -51,8 +50,13 @@
     }
 
     QPair<QUuid, QString> getSecurePath(QString connectionName) {
+        QUuid id = _uuidFactory->createUuid();
+        return getSecurePath(id, connectionName);
+    }
+
+    QPair<QUuid, QString> getSecurePath(QUuid id, QString connectionName) {
         if (connectionName.isEmpty()) {
-            return getUuidString(QString(BASE_ACCOUNT_URL));
+            return getUuidString(id, QString(BASE_ACCOUNT_URL));
         }
 
         QDBusPendingReply<QString> reply =
@@ -61,14 +65,14 @@
         reply.waitForFinished();
         if (reply.isError()) {
             qCritical() << reply.error();
-            return getUuidString(QString(BASE_ACCOUNT_URL));
+            return getUuidString(id, QString(BASE_ACCOUNT_URL));
         } else {
             // use the returned value
             QString appId = reply.value();
             qDebug() << "AppId is " << appId;
 
             if (appId.isEmpty() || appId == UNCONFINED_ID) {
-                return getUuidString(QString(BASE_ACCOUNT_URL));
+                return getUuidString(id, QString(BASE_ACCOUNT_URL));
             } else {
                 QByteArray appIdBa = appId.toUtf8();
 
@@ -77,17 +81,18 @@
                     appIdBa.data(), NULL);
 
                 if (appIdPath == NULL) {
-                    qCritical() << "Unable to allocate memory for nih_dbus_path()";
-                    return getUuidString(QString(BASE_ACCOUNT_URL));
+                    qCritical() << "Unable to allocate memory for "
+                        << "nih_dbus_path()";
+                    return getUuidString(id, QString(BASE_ACCOUNT_URL));
                 }
                 QString path = QString(appIdPath);
                 qDebug() << "AppId path is " << appIdPath;
 
                 // free nih data
                 nih_free(appIdPath);
-                return getUuidString(path);
-            } // not empty appid string
-        } // no dbus error
+                return getUuidString(id, path);
+            }  // not empty appid string
+        }  // no dbus error
     }
 
  private:
@@ -116,3 +121,9 @@
     Q_D(AppArmor);
     return d->getSecurePath(connectionName);
 }
+
+QPair<QUuid, QString>
+AppArmor::getSecurePath(QUuid id, QString connectionName) {
+    Q_D(AppArmor);
+    return d->getSecurePath(id, connectionName);
+}

=== modified file 'libubuntudownloadmanager/apparmor.h'
--- libubuntudownloadmanager/apparmor.h	2013-09-10 15:36:23 +0000
+++ libubuntudownloadmanager/apparmor.h	2013-09-10 15:36:23 +0000
@@ -33,6 +33,7 @@
     explicit AppArmor(QObject *parent = 0);
 
     virtual QPair<QUuid, QString> getSecurePath(QString connName);
+    virtual QPair<QUuid, QString> getSecurePath(QUuid id, QString connName);
 
  private:
     // use pimpl so that we can mantains ABI compatibility

=== modified file 'libubuntudownloadmanager/download_factory.cpp'
--- libubuntudownloadmanager/download_factory.cpp	2013-09-10 15:36:23 +0000
+++ libubuntudownloadmanager/download_factory.cpp	2013-09-10 15:36:23 +0000
@@ -23,6 +23,7 @@
 #include "./single_download.h"
 #include "./download_factory.h"
 
+#define OBJECT_PATH_KEY "objectpath"
 /*
  * PRIVATE IMPLEMENTATION
  */
@@ -45,11 +46,30 @@
           q_ptr(parent) {
     }
 
+    QPair<QUuid, QString> getDownloadPath(const QString& dbusOwner,
+                                          const QVariantMap& metadata) {
+        qDebug() << __PRETTY_FUNCTION__ << dbusOwner << metadata;
+        if (metadata.contains(OBJECT_PATH_KEY)) {
+            // create a uuid using the string value form the metadata
+            QUuid id = QUuid(metadata[OBJECT_PATH_KEY].toString());
+            if (id.isNull()) {
+                qCritical() << "Uuid sent by client is NULL";
+                return _apparmor->getSecurePath(dbusOwner);
+            } else {
+                qDebug() << "Using the id from the client" << id;
+                return _apparmor->getSecurePath(id, dbusOwner);
+            }
+        } else {
+            qDebug() << "DownloadFactory assigns the Download Uuid.";
+            return _apparmor->getSecurePath(dbusOwner);
+        }
+    }
+
     Download* createDownload(const QString& dbusOwner,
                              const QUrl& url,
                              const QVariantMap& metadata,
                              const QMap<QString, QString>& headers) {
-        QPair<QUuid, QString> idData = _apparmor->getSecurePath(dbusOwner);
+        QPair<QUuid, QString> idData = getDownloadPath(dbusOwner, metadata);
         Download* down = new SingleDownload(idData.first, idData.second, url,
             metadata, headers, _networkInfo, _nam, _processFactory);
         DownloadAdaptor* adaptor = new DownloadAdaptor(down);
@@ -63,7 +83,7 @@
                              QCryptographicHash::Algorithm algo,
                              const QVariantMap& metadata,
                              const QMap<QString, QString>& headers) {
-        QPair<QUuid, QString> idData = _apparmor->getSecurePath(dbusOwner);
+        QPair<QUuid, QString> idData = getDownloadPath(dbusOwner, metadata);
         Download* down = new SingleDownload(idData.first, idData.second, url,
             hash, algo, metadata, headers, _networkInfo, _nam,
             _processFactory);
@@ -78,7 +98,7 @@
                              bool allowed3G,
                              const QVariantMap& metadata,
                              StringMap headers) {
-        QPair<QUuid, QString> idData = _apparmor->getSecurePath(dbusOwner);
+        QPair<QUuid, QString> idData = getDownloadPath(dbusOwner, metadata);
         Download* down = new GroupDownload(idData.first, idData.second,
             downloads, algo, allowed3G, metadata, headers, _networkInfo,
             _self);

=== modified file 'ubuntu-download-manager-tests/fake_apparmor.cpp'
--- ubuntu-download-manager-tests/fake_apparmor.cpp	2013-09-10 15:36:23 +0000
+++ ubuntu-download-manager-tests/fake_apparmor.cpp	2013-09-10 15:36:23 +0000
@@ -42,3 +42,22 @@
 
     return QPair<QUuid, QString>(id, uuidString);
 }
+
+QPair<QUuid, QString>
+FakeAppArmor::getSecurePath(QUuid id, QString connName) {
+    QString uuidString = id.toString().replace(QRegExp("[-{}]"), "");
+    if (_recording) {
+        QList<QObject*> inParams;
+        inParams.append(new UuidWrapper(id));
+        inParams.append(new StringWrapper(connName));
+
+        QList<QObject*> outParams;
+        outParams.append(new UuidWrapper(id));
+        outParams.append(new StringWrapper(uuidString));
+        MethodParams params(inParams, outParams);
+        MethodData methodData("getSecurePath", params);
+        _called.append(methodData);
+    }
+
+    return QPair<QUuid, QString>(id, uuidString);
+}

=== modified file 'ubuntu-download-manager-tests/fake_apparmor.h'
--- ubuntu-download-manager-tests/fake_apparmor.h	2013-09-10 15:36:23 +0000
+++ ubuntu-download-manager-tests/fake_apparmor.h	2013-09-10 15:36:23 +0000
@@ -32,6 +32,7 @@
                  QObject *parent = 0);
 
     QPair<QUuid, QString> getSecurePath(QString connName) override;
+    QPair<QUuid, QString> getSecurePath(QUuid id, QString connName) override;
 
  private:
     QSharedPointer<UuidFactory> _uuidFactory;

=== modified file 'ubuntu-download-manager-tests/test_download_factory.cpp'
--- ubuntu-download-manager-tests/test_download_factory.cpp	2013-09-10 15:36:23 +0000
+++ ubuntu-download-manager-tests/test_download_factory.cpp	2013-09-10 15:36:23 +0000
@@ -105,3 +105,149 @@
     QCOMPARE(download->downloadId(), id->value());
     QCOMPARE(download->path(), path->value());
 }
+
+void
+TestDownloadFactory::testCreateDownloadWithValidUuid() {
+    _apparmor->record();
+
+    // create a download, assert that it was
+    // created and that the id and the path are correctly set
+    QUuid id = QUuid::createUuid();
+
+    QVariantMap metadata;
+    metadata["objectpath"] = id.toString();
+
+    Download* download = _downFactory->createDownload("", QUrl(),
+        metadata, QMap<QString, QString>());
+
+    QList<MethodData> calledMethods = _apparmor->calledMethods();
+    QCOMPARE(1, calledMethods.count());
+    StringWrapper* path = reinterpret_cast<StringWrapper*>(
+        calledMethods[0].params().outParams()[1]);
+    QCOMPARE(download->downloadId(), id);
+    QCOMPARE(download->path(), path->value());
+}
+
+void
+TestDownloadFactory::testCreateDownloadWithNullUuid() {
+    _apparmor->record();
+
+    // create a download, assert that it was
+    // created and that the id and the path are correctly set
+    QVariantMap metadata;
+    metadata["objectpath"] = "bad-id";
+
+    Download* download = _downFactory->createDownload("", QUrl(),
+        QVariantMap(), QMap<QString, QString>());
+
+    QList<MethodData> calledMethods = _apparmor->calledMethods();
+    QCOMPARE(1, calledMethods.count());
+    UuidWrapper* id = reinterpret_cast<UuidWrapper*>(
+        calledMethods[0].params().outParams()[0]);
+    StringWrapper* path = reinterpret_cast<StringWrapper*>(
+        calledMethods[0].params().outParams()[1]);
+    QCOMPARE(download->downloadId(), id->value());
+    QCOMPARE(download->path(), path->value());
+}
+
+void
+TestDownloadFactory::testCreateDownloadWithHashAndUuid() {
+    _apparmor->record();
+
+    QUuid id = QUuid::createUuid();
+
+    QVariantMap metadata;
+    metadata["objectpath"] = id.toString();
+
+    QString hash = "my-hash";
+    QCryptographicHash::Algorithm algo = QCryptographicHash::Md5;
+
+    // same as above but assert hash and hash algo
+    Download* download = _downFactory->createDownload("", QUrl(),
+        hash, algo, metadata, QMap<QString, QString>());
+
+    QList<MethodData> calledMethods = _apparmor->calledMethods();
+    QCOMPARE(1, calledMethods.count());
+    StringWrapper* path = reinterpret_cast<StringWrapper*>(
+        calledMethods[0].params().outParams()[1]);
+    QCOMPARE(download->downloadId(), id);
+    QCOMPARE(download->path(), path->value());
+
+    SingleDownload* single = reinterpret_cast<SingleDownload*>(download);
+    QCOMPARE(hash, single->hash());
+    QCOMPARE(algo, single->hashAlgorithm());
+}
+
+void
+TestDownloadFactory::testCreateDownloadWithHashAndNullUuid() {
+    _apparmor->record();
+
+    QVariantMap metadata;
+    metadata["objectpath"] = "bad-id";
+
+    QString hash = "my-hash";
+    QCryptographicHash::Algorithm algo = QCryptographicHash::Md5;
+
+    // same as above but assert hash and hash algo
+    Download* download = _downFactory->createDownload("", QUrl(),
+        hash, algo, metadata, QMap<QString, QString>());
+
+    QList<MethodData> calledMethods = _apparmor->calledMethods();
+    QCOMPARE(1, calledMethods.count());
+    UuidWrapper* id = reinterpret_cast<UuidWrapper*>(
+        calledMethods[0].params().outParams()[0]);
+    StringWrapper* path = reinterpret_cast<StringWrapper*>(
+        calledMethods[0].params().outParams()[1]);
+    QCOMPARE(download->downloadId(), id->value());
+    QCOMPARE(download->path(), path->value());
+
+    SingleDownload* single = reinterpret_cast<SingleDownload*>(download);
+    QCOMPARE(hash, single->hash());
+    QCOMPARE(algo, single->hashAlgorithm());
+}
+
+void
+TestDownloadFactory::testCreateGroupDownloadWithValidUuid() {
+    _apparmor->record();
+
+    // create a download, assert that it was
+    // created and that the id and the path are correctly set
+    QUuid id = QUuid::createUuid();
+
+    QVariantMap metadata;
+    metadata["objectpath"] = id.toString();
+
+    Download* download = _downFactory->createDownload("",
+        QList<GroupDownloadStruct>(), QCryptographicHash::Md5,
+        true, metadata, QMap<QString, QString>());
+
+    QList<MethodData> calledMethods = _apparmor->calledMethods();
+    QCOMPARE(1, calledMethods.count());
+    StringWrapper* path = reinterpret_cast<StringWrapper*>(
+        calledMethods[0].params().outParams()[1]);
+    QCOMPARE(download->downloadId(), id);
+    QCOMPARE(download->path(), path->value());
+}
+
+void
+TestDownloadFactory::testCreateGroupDownloadWithNullUuid() {
+    _apparmor->record();
+
+    // create a download, assert that it was
+    // created and that the id and the path are correctly set
+    QVariantMap metadata;
+    metadata["objectpath"] = "bad-id";
+
+    Download* download = _downFactory->createDownload("",
+        QList<GroupDownloadStruct>(), QCryptographicHash::Md5,
+        true, metadata, QMap<QString, QString>());
+
+    QList<MethodData> calledMethods = _apparmor->calledMethods();
+    QCOMPARE(1, calledMethods.count());
+    UuidWrapper* id = reinterpret_cast<UuidWrapper*>(
+        calledMethods[0].params().outParams()[0]);
+    StringWrapper* path = reinterpret_cast<StringWrapper*>(
+        calledMethods[0].params().outParams()[1]);
+    QCOMPARE(download->downloadId(), id->value());
+    QCOMPARE(download->path(), path->value());
+}

=== modified file 'ubuntu-download-manager-tests/test_download_factory.h'
--- ubuntu-download-manager-tests/test_download_factory.h	2013-09-10 15:36:23 +0000
+++ ubuntu-download-manager-tests/test_download_factory.h	2013-09-10 15:36:23 +0000
@@ -30,6 +30,7 @@
 
 class TestDownloadFactory : public QObject {
     Q_OBJECT
+
  public:
     explicit TestDownloadFactory(QObject *parent = 0);
 
@@ -41,6 +42,12 @@
     void testCreateDownload();
     void testCreateDownloadWithHash();
     void testCreateGroupDownload();
+    void testCreateDownloadWithValidUuid();
+    void testCreateDownloadWithNullUuid();
+    void testCreateDownloadWithHashAndUuid();
+    void testCreateDownloadWithHashAndNullUuid();
+    void testCreateGroupDownloadWithValidUuid();
+    void testCreateGroupDownloadWithNullUuid();
 
  private:
     FakeAppArmor* _apparmor;