Merge lp:~marcustomlinson/unity-scope-click/lp-1578283 into lp:unity-scope-click

[Marcus Tomlinson]

Revert recent landing of: https://code.launchpad.net/~dobey/unity-scope-click/really-we-want-to-sign-all-the-requests/+merge/290763

[Marcus Tomlinson (marcustomlinson)]

This is in silo 54 at the moment. So far, those who have experienced the blank apps scope have been unable to reproduce with this applied.

I wish I could reproduce the issue myself so that I can better understand why this would fix it :/

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:444
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/742/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/146/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/142/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/742/rebuild

review: Needs Fixing (continuous-integration)

[Paweł Stołowski (stolowski)]

+1

review: Approve

[Marcus Tomlinson (marcustomlinson)]

Rejecting in favour of: https://code.launchpad.net/~marcustomlinson/unity-scope-click/lp-1578283-2/+merge/294246

review: Disapprove

Unmerged revisions

=== modified file 'libclickscope/click/download-manager.cpp'
--- libclickscope/click/download-manager.cpp	2016-04-15 06:43:36 +0000
+++ libclickscope/click/download-manager.cpp	2016-05-09 17:12:23 +0000
@@ -154,11 +154,11 @@
                 });
     QObject::connect(response.data(), &click::web::Response::error,
                      [this, callback, package_name](QString error, int error_code) {
-                         qWarning() << QStringLiteral("Network error (%1) fetching click token for:").arg(error_code) << package_name.c_str();
+                         qDebug() << QStringLiteral("Network error (%1) fetching click token for:").arg(error_code) << package_name.c_str();
                          switch(error_code) {
                          case 401:
                          case 403:
-                             client->invalidateCredentials();
+                             sso->invalidateCredentials();
                              callback(error.toUtf8().data(), Error::CredentialsError);
                              break;
                          default:
@@ -172,6 +172,7 @@
 void DownloadManager::setCredentialsService(const QSharedPointer<click::CredentialsService>& credentialsService)
 {
     sso = credentialsService;
+    client->setCredentialsService(sso);
 }
 
 } // namespace click

=== modified file 'libclickscope/click/pay.cpp'
--- libclickscope/click/pay.cpp	2016-04-01 17:16:05 +0000
+++ libclickscope/click/pay.cpp	2016-05-09 17:12:23 +0000
@@ -228,6 +228,9 @@
 
 click::web::Cancellable Package::get_purchases(std::function<void(const PurchaseSet&)> callback)
 {
+    QSharedPointer<click::CredentialsService> sso(new click::CredentialsService());
+    client->setCredentialsService(sso);
+
     QSharedPointer<click::web::Response> response = client->call
         (get_base_url() + pay::API_ROOT + pay::PURCHASES_API_PATH, "GET", true);
 

=== modified file 'libclickscope/click/preview.cpp'
--- libclickscope/click/preview.cpp	2016-04-01 17:16:05 +0000
+++ libclickscope/click/preview.cpp	2016-05-09 17:12:23 +0000
@@ -734,6 +734,8 @@
     std::promise<bool> promise;
     auto future = promise.get_future();
     run_under_qt([this, reply, &promise]() {
+            QSharedPointer<click::CredentialsService> sso(new click::CredentialsService());
+            dm->setCredentialsService(sso);
             dm->start(download_url, download_sha512, result["name"].get_string(),
                       [this, reply, &promise] (std::string msg, DownloadManager::Error dmerr){
                           switch (dmerr)
@@ -886,6 +888,8 @@
             std::promise<bool> submit_promise;
             std::future<bool> submit_future = submit_promise.get_future();
             qt::core::world::enter_with_task([this, review, &submit_promise, widget_id]() mutable {
+                    QSharedPointer<click::CredentialsService> sso(new click::CredentialsService());
+                    client->setCredentialsService(sso);
                     if (widget_id == "rating") {
                         submit_operation = reviews->submit_review(review,
                                                               [&submit_promise](click::Reviews::Error){

=== modified file 'libclickscope/click/reviews.cpp'
--- libclickscope/click/reviews.cpp	2016-04-01 17:16:05 +0000
+++ libclickscope/click/reviews.cpp	2016-05-09 17:12:23 +0000
@@ -131,8 +131,7 @@
     click::web::CallParams params;
     params.add(click::REVIEWS_QUERY_ARGNAME, package_name.c_str());
     QSharedPointer<click::web::Response> response = client->call
-        (get_base_url() + click::REVIEWS_API_PATH, "GET", false,
-         std::map<std::string, std::string>{}, "", params);
+        (get_base_url() + click::REVIEWS_API_PATH, params);
 
     QObject::connect(response.data(), &click::web::Response::finished,
                 [=](QString reply) {

=== modified file 'libclickscope/click/ubuntuone_credentials.cpp'
--- libclickscope/click/ubuntuone_credentials.cpp	2016-04-01 17:16:05 +0000
+++ libclickscope/click/ubuntuone_credentials.cpp	2016-05-09 17:12:23 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014-2016 Canonical Ltd.
+ * Copyright (C) 2014 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 3, as published
@@ -29,9 +29,6 @@
 
 #include "ubuntuone_credentials.h"
 
-#include <future>
-#include <QCoreApplication>
-
 namespace u1 = UbuntuOne;
 
 click::CredentialsService::CredentialsService()
@@ -50,43 +47,6 @@
 {
 }
 
-UbuntuOne::Token click::CredentialsService::getToken()
-{
-    if (!_token.isValid()) {
-        std::promise<UbuntuOne::Token> promise;
-        auto future = promise.get_future();
-
-        auto success = QObject::connect(ssoService.data(),
-                                        &u1::SSOService::credentialsFound,
-                                        [this, &promise](const u1::Token& token) {
-                                            emit credentialsFound(_token);
-                                            promise.set_value(token);
-                                        });
-        auto notfound = QObject::connect(ssoService.data(),
-                                         &u1::SSOService::credentialsNotFound,
-                                         [this, &promise]() {
-                                             qWarning() << "No Ubuntu One token found.";
-                                             emit credentialsNotFound();
-                                             promise.set_value(u1::Token());
-                                         });
-
-        getCredentials();
-
-        std::future_status status = future.wait_for(std::chrono::milliseconds(0));
-        while (status != std::future_status::ready) {
-            QCoreApplication::processEvents();
-            qDebug() << "Processed some events, waiting to process again.";
-            status = future.wait_for(std::chrono::milliseconds(100));
-        }
-
-        _token = future.get();
-        QObject::disconnect(success);
-        QObject::disconnect(notfound);
-    }
-
-    return _token;
-}
-
 void click::CredentialsService::getCredentials()
 {
     ssoService->getCredentials();

=== modified file 'libclickscope/click/ubuntuone_credentials.h'
--- libclickscope/click/ubuntuone_credentials.h	2016-04-01 17:16:05 +0000
+++ libclickscope/click/ubuntuone_credentials.h	2016-05-09 17:12:23 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014-2016 Canonical Ltd.
+ * Copyright (C) 2014 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 3, as published
@@ -47,7 +47,6 @@
     
     CredentialsService& operator=(const CredentialsService&) = delete;
 
-    virtual UbuntuOne::Token getToken();
     virtual void getCredentials();
     virtual void invalidateCredentials();
 
@@ -58,7 +57,7 @@
 
 private:
     QScopedPointer<UbuntuOne::SSOService> ssoService;
-    UbuntuOne::Token _token;
+
 }; // CredentialsService
 
 } // namespace click

=== modified file 'libclickscope/click/webclient.cpp'
--- libclickscope/click/webclient.cpp	2016-04-01 17:16:05 +0000
+++ libclickscope/click/webclient.cpp	2016-05-09 17:12:23 +0000
@@ -82,7 +82,7 @@
     const std::string& iri,
     const click::web::CallParams& params)
 {
-    return call(iri, "GET", true,
+    return call(iri, "GET", false,
                 std::map<std::string, std::string>(), "", params);
 }
 
@@ -132,21 +132,25 @@
     request->setRawHeader(DEVICE_ID_HEADER.c_str(), deviceId.data());
 
     if (sign && !impl->sso.isNull()) {
-        auto token = impl->sso->getToken();
-        if (token.isValid()) {
-            QString auth_header = token.signUrl(url.toString(),
-                                                method.c_str());
-            qDebug() << "Signed URL:" << request->url().toString();
-            request->setRawHeader(AUTHORIZATION_HEADER.c_str(), auth_header.toUtf8());
-        } else {
-            qWarning() << "Signing reuested but returned token is invalid.";
-        }
-
-        doConnect();
+        click::utils::SmartConnect sc(responsePtr.data());
+        sc.connect(impl->sso.data(), &click::CredentialsService::credentialsFound,
+                   [=](const UbuntuOne::Token& token) {
+                       QString auth_header = token.signUrl(url.toString(),
+                                                           method.c_str());
+                       qDebug() << "Signed URL:" << request->url().toString();
+                       request->setRawHeader(AUTHORIZATION_HEADER.c_str(), auth_header.toUtf8());
+                       impl->sso.clear();
+                       doConnect();
+                   });
+        sc.connect(impl->sso.data(), &click::CredentialsService::credentialsNotFound,
+                   [=]() {
+                       impl->sso.clear();
+                       qWarning() << "Signing reuested but no credentials found. Using unsigned URL.";
+                       doConnect();
+                   });
+        // TODO: Need to handle error signal once in CredentialsService.
+        impl->sso->getCredentials();
     } else {
-        if (sign && impl->sso.isNull()) {
-            qCritical() << "Unable to sign request without SSO object.";
-        }
         doConnect();
     }
 

=== modified file 'libclickscope/click/webclient.h'
--- libclickscope/click/webclient.h	2016-04-01 17:16:05 +0000
+++ libclickscope/click/webclient.h	2016-05-09 17:12:23 +0000
@@ -121,7 +121,7 @@
     virtual QSharedPointer<Response> call(
         const std::string& iri,
         const std::string& method,
-        bool sign = true,
+        bool sign = false,
         const std::map<std::string, std::string>& headers = std::map<std::string, std::string>(),
         const std::string& data = "",
         const CallParams& params = CallParams());

=== modified file 'libclickscope/tests/mock_ubuntuone_credentials.h'
--- libclickscope/tests/mock_ubuntuone_credentials.h	2016-04-01 17:16:05 +0000
+++ libclickscope/tests/mock_ubuntuone_credentials.h	2016-05-09 17:12:23 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014-2016 Canonical Ltd.
+ * Copyright (C) 2014 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 3, as published
@@ -27,11 +27,11 @@
  * files in the program, then also delete it here.
  */
 
-#include <token.h>
 
 class MockCredentialsService : public click::CredentialsService {
  public:
-    MOCK_METHOD0(getToken, UbuntuOne::Token());
-    MOCK_METHOD0(getCredentials, void());
-    MOCK_METHOD0(invalidateCredentials, void());
+  MOCK_METHOD0(getCredentials,
+      void());
+  MOCK_METHOD0(invalidateCredentials,
+      void());
 };

=== modified file 'libclickscope/tests/mock_webclient.h'
--- libclickscope/tests/mock_webclient.h	2016-04-01 17:16:05 +0000
+++ libclickscope/tests/mock_webclient.h	2016-05-09 17:12:23 +0000
@@ -87,13 +87,13 @@
     QSharedPointer<click::web::Response> call(
         const std::string& iri,
         const click::web::CallParams& params=click::web::CallParams()) override {
-        return callImpl(iri, "GET", true,
+        return callImpl(iri, "GET", false,
                         std::map<std::string, std::string>(), "", params);
     }
     QSharedPointer<click::web::Response> call(
         const std::string& iri,
         const std::string& method,
-        bool sign = true,
+        bool sign = false,
         const std::map<std::string, std::string>& headers = std::map<std::string, std::string>(),
         const std::string& data = "",
         const click::web::CallParams& params=click::web::CallParams()) override {
@@ -102,6 +102,7 @@
 
     MOCK_METHOD1(has_header, bool(const std::string& header));
     MOCK_METHOD1(get_header, std::string(const std::string&header));
+    MOCK_METHOD0(invalidateCredentials, void());
 };
 
 }

=== modified file 'libclickscope/tests/test_download_manager.cpp'
--- libclickscope/tests/test_download_manager.cpp	2016-04-01 17:16:05 +0000
+++ libclickscope/tests/test_download_manager.cpp	2016-05-09 17:12:23 +0000
@@ -63,7 +63,6 @@
 
     virtual void SetUp()
     {
-        ssoPtr.reset(new MockCredentialsService());
         namPtr.reset(new MockNetworkAccessManager());
         clientPtr.reset(new NiceMock<MockClient>(namPtr));
         clientPtr->setCredentialsService(ssoPtr);
@@ -161,6 +160,9 @@
     LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
     auto response = responseForReply(reply.asSharedPtr());
 
+    QSharedPointer<MockCredentialsService> sso(new MockCredentialsService());
+    dmPtr->setCredentialsService(sso);
+
     EXPECT_CALL(reply.instance, errorString())
         .WillOnce(Return(QString("ERROR")));
     EXPECT_CALL(reply.instance, attribute(_)).WillOnce(Return(QVariant(401)));
@@ -170,7 +172,7 @@
     EXPECT_CALL(*clientPtr, callImpl(_, _, _, _, _, _))
             .Times(1)
             .WillOnce(Return(response));
-    EXPECT_CALL(*ssoPtr, invalidateCredentials());
+    EXPECT_CALL(*(sso.data()), invalidateCredentials());
     EXPECT_CALL(*this, start_callback("ERROR (201)",
                                       click::DownloadManager::Error::CredentialsError)).Times(1);
 

=== modified file 'libclickscope/tests/test_index.cpp'
--- libclickscope/tests/test_index.cpp	2016-04-01 17:16:05 +0000
+++ libclickscope/tests/test_index.cpp	2016-05-09 17:12:23 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014-2016 Canonical Ltd.
+ * Copyright (C) 2014 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 3, as published
@@ -190,18 +190,6 @@
     indexPtr->departments("departments", [](const click::DepartmentList&, const click::HighlightList&, click::Index::Error, int) {});
 }
 
-TEST_F(IndexTest, testDetailsSignsCall)
-{
-    LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
-    auto response = responseForReply(reply.asSharedPtr());
-
-    EXPECT_CALL(*clientPtr, callImpl(_, _, true, _, _, _))
-            .Times(1)
-            .WillOnce(Return(response));
-
-    indexPtr->get_details("fake-app", [](const click::PackageDetails, click::Index::Error) {});
-}
-
 TEST_F(IndexTest, testSearchSendsRightPath)
 {
     LifetimeHelper<click::network::Reply, MockNetworkReply> reply;

=== modified file 'libclickscope/tests/test_reviews.cpp'
--- libclickscope/tests/test_reviews.cpp	2016-04-01 17:16:05 +0000
+++ libclickscope/tests/test_reviews.cpp	2016-05-09 17:12:23 +0000
@@ -150,19 +150,6 @@
                                      click::Reviews::Error) {});
 }
 
-TEST_F(ReviewsTest, testFetchReviewsDoesNotSignCall)
-{
-    LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
-    auto response = responseForReply(reply.asSharedPtr());
-
-    EXPECT_CALL(*clientPtr, callImpl(_, _, false, _, _, _))
-            .Times(1)
-            .WillOnce(Return(response));
-
-    reviewsPtr->fetch_reviews("", [](click::ReviewList,
-                                     click::Reviews::Error) {});
-}
-
 TEST_F(ReviewsTest, testFetchReviewsSendsQueryAsParam)
 {
     LifetimeHelper<click::network::Reply, MockNetworkReply> reply;

=== modified file 'libclickscope/tests/test_webclient.cpp'
--- libclickscope/tests/test_webclient.cpp	2016-04-01 17:16:05 +0000
+++ libclickscope/tests/test_webclient.cpp	2016-05-09 17:12:23 +0000
@@ -221,9 +221,11 @@
     click::web::Client wc(namPtr);
     wc.setCredentialsService(ssoPtr);
 
-    EXPECT_CALL(sso, getToken())
-        .WillOnce(Return(UbuntuOne::Token("token_key", "token_secret",
-                                          "consumer_key", "consumer_secret")));
+    EXPECT_CALL(sso, getCredentials()).WillOnce(Invoke([&](){
+                UbuntuOne::Token token("token_key", "token_secret",
+                                       "consumer_key", "consumer_secret");
+                sso.credentialsFound(token);
+            }));
     EXPECT_CALL(nam, sendCustomRequest(IsValidOAuthHeader(true), _, _))
             .Times(1)
             .WillOnce(Return(replyPtr));
@@ -269,7 +271,9 @@
     click::web::Client wc(namPtr);
     wc.setCredentialsService(ssoPtr);
 
-    EXPECT_CALL(sso, getToken()).WillOnce(Return(UbuntuOne::Token()));
+    EXPECT_CALL(sso, getCredentials()).WillOnce(Invoke([&]() {
+                sso.credentialsNotFound();
+            }));
     EXPECT_CALL(nam, sendCustomRequest(IsValidOAuthHeader(false), _, _))
         .Times(1)
         .WillOnce(Return(replyPtr));

=== modified file 'scope/clickapps/apps-scope.cpp'
--- scope/clickapps/apps-scope.cpp	2016-04-15 09:24:18 +0000
+++ scope/clickapps/apps-scope.cpp	2016-05-09 17:12:23 +0000
@@ -82,8 +82,6 @@
     static const int zero = 0;
     auto emptyCb = [this]()
     {
-        sso.reset(new click::CredentialsService());
-        client->setCredentialsService(sso);
         dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());
         qt_ready_p.set_value();
     };

=== modified file 'scope/clickapps/apps-scope.h'
--- scope/clickapps/apps-scope.h	2016-04-15 09:24:18 +0000
+++ scope/clickapps/apps-scope.h	2016-05-09 17:12:23 +0000
@@ -75,7 +75,6 @@
     QSharedPointer<click::Index> index;
     QSharedPointer<pay::Package> pay_package;
     QSharedPointer<Ubuntu::DownloadManager::Manager> dm;
-    QSharedPointer<click::CredentialsService> sso;
     std::shared_ptr<click::DepartmentsDb> depts_db;
 
     std::string installApplication(unity::scopes::Result const& result);

=== modified file 'scope/clickstore/store-scope.cpp'
--- scope/clickstore/store-scope.cpp	2016-04-15 09:24:18 +0000
+++ scope/clickstore/store-scope.cpp	2016-05-09 17:12:23 +0000
@@ -84,8 +84,6 @@
     static const int zero = 0;
     auto emptyCb = [this]()
     {
-        sso.reset(new click::CredentialsService());
-        client->setCredentialsService(sso);
         dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());
         qt_ready_p.set_value();
     };

=== modified file 'scope/clickstore/store-scope.h'
--- scope/clickstore/store-scope.h	2016-04-15 09:24:18 +0000
+++ scope/clickstore/store-scope.h	2016-05-09 17:12:23 +0000
@@ -78,7 +78,6 @@
     QSharedPointer<click::Index> index;
     QSharedPointer<pay::Package> pay_package;
     QSharedPointer<Ubuntu::DownloadManager::Manager> dm;
-    QSharedPointer<click::CredentialsService> sso;
     std::shared_ptr<click::DepartmentLookup> depts;
     std::shared_ptr<click::HighlightList> highlights;
     std::shared_ptr<click::DepartmentsDb> depts_db;