Merge lp:~mardy/ubuntu-system-settings-online-accounts/empty-page-1594944 into lp:ubuntu-system-settings-online-accounts

[Alberto Mardegan]

Properly handle early termination of account access requests

When a degenerate request (coming from an unconfined process with empty application ID) was received, the allowed() signal was emitted from the Component.onCompleted handler.
However, given that this all was happening inside the QQuickView::setSource() method, the signal was effectively emitted before the C++ code had a chance to setup the signal connections, with the result that such a signal was lost.

To solve this, we export a "request" object to QML, whose methods replace the old signals.

Properly handle early termination of account access requests

When a degenerate request (coming from an unconfined process with empty application ID) was received, the allowed() signal was emitted from the Component.onCompleted handler.
However, given that this all was happening inside the QQuickView::setSource() method, the signal was effectively emitted before the C++ code had a chance to setup the signal connections, with the result that such a signal was lost.

To solve this, we export a "request" object to QML, whose methods replace the old signals.

[dobey (dobey)]

I've build this in the silo where I have the ubuntuone-credentials changes, and a blank window is still opened, so this doesn't seem to resolve the bug.

Also, I was trying to find where this file exists on disk, so I could edit in place instead of building a silo, given it's a trivial change, however I do not see this file being installed to the system at all. Is it only included as a resource in Qt?

review: Needs Fixing

[lp:~mardy/ubuntu-system-settings-online-accounts/empty-page-1594944]

368. By Alberto Mardegan on 2016-06-23

Expose request to QML

369. By Alberto Mardegan on 2016-06-23

Avoid using context properties

370. By Alberto Mardegan on 2016-06-23

Don't connect to inexistent signal

371. By Alberto Mardegan on 2016-06-23

revert

372. By Alberto Mardegan on 2016-06-23

Update tst_provider_request

[Alberto Mardegan (mardy)]

Update.

=== modified file 'online-accounts-ui/provider-request.cpp'
--- online-accounts-ui/provider-request.cpp	2015-11-18 08:27:12 +0000
+++ online-accounts-ui/provider-request.cpp	2016-06-23 13:59:19 +0000
@@ -41,6 +41,8 @@
 {
     Q_OBJECT
     Q_DECLARE_PUBLIC(ProviderRequest)
+    Q_PROPERTY(QVariantMap application READ applicationInfo CONSTANT)
+    Q_PROPERTY(QVariantMap provider READ providerInfo CONSTANT)
 
 public:
     ProviderRequestPrivate(ProviderRequest *request);
@@ -48,18 +50,21 @@
 
     void start();
 
+    QVariantMap applicationInfo() const { return m_applicationInfo; }
+    QVariantMap providerInfo() const { return m_providerInfo; }
+
+public Q_SLOTS:
+    void deny();
+    void allow(int accountId);
+
 private Q_SLOTS:
     void onWindowVisibleChanged(bool visible);
-    void onDenied();
-    void onAllowed(int accountId);
-
-private:
-    QVariantMap providerInfo(const QString &providerId) const;
 
 private:
     mutable ProviderRequest *q_ptr;
     QQuickView *m_view;
     QVariantMap m_applicationInfo;
+    QVariantMap m_providerInfo;
 };
 
 } // namespace
@@ -112,7 +117,7 @@
     } else {
         providerId = q->parameters().value(OAU_KEY_PROVIDER).toString();
     }
-    QVariantMap providerInfo = appManager->providerInfo(providerId);
+    m_providerInfo = appManager->providerInfo(providerId);
 
     m_view = new QQuickView;
     QObject::connect(m_view, SIGNAL(visibleChanged(bool)),
@@ -126,7 +131,7 @@
      *   <package-dir>/lib/<DEB_HOST_MULTIARCH>
      * to the QML import path.
      */
-    QString packageDir = providerInfo.value("package-dir").toString();
+    QString packageDir = m_providerInfo.value("package-dir").toString();
     if (!packageDir.isEmpty()) {
         engine->addImportPath(packageDir + "/lib");
 #ifdef DEB_HOST_MULTIARCH
@@ -142,22 +147,15 @@
                                 QUrl::fromLocalFile(QStandardPaths::writableLocation(
                                     QStandardPaths::GenericDataLocation) +
                                 "/accounts/qml-plugins/"));
-    context->setContextProperty("provider", providerInfo);
-    context->setContextProperty("application", m_applicationInfo);
+    context->setContextProperty("request", this);
     context->setContextProperty("mainWindow", m_view);
 
     m_view->setSource(QUrl(QStringLiteral("qrc:/qml/ProviderRequest.qml")));
-    QQuickItem *root = m_view->rootObject();
-    QObject::connect(root, SIGNAL(denied()),
-                     this, SLOT(onDenied()));
-    QObject::connect(root, SIGNAL(allowed(int)),
-                     this, SLOT(onAllowed(int)));
-    if (root->property("wasDenied").toBool()) {
-        DEBUG() << "Was denied";
-        q->setResult(QVariantMap());
-        return;
+    /* It could be that allow() or deny() have been already called; don't show
+     * the window in that case. */
+    if (q->isInProgress()) {
+        q->setWindow(m_view);
     }
-    q->setWindow(m_view);
 }
 
 void ProviderRequestPrivate::onWindowVisibleChanged(bool visible)
@@ -171,22 +169,27 @@
     }
 }
 
-void ProviderRequestPrivate::onDenied()
+void ProviderRequestPrivate::deny()
 {
+    Q_Q(ProviderRequest);
     DEBUG();
-    /* Just close the window; this will deliver the empty result to the
-     * client */
-    m_view->close();
+    if (m_view->isVisible()) {
+        /* Just close the window; this will deliver the empty result to the
+         * client */
+        m_view->close();
+    } else {
+        q->setResult(QVariantMap());
+    }
 }
 
-void ProviderRequestPrivate::onAllowed(int accountId)
+void ProviderRequestPrivate::allow(int accountId)
 {
     Q_Q(ProviderRequest);
     DEBUG() << "Access allowed for account:" << accountId;
     /* If the request came from an app, add a small delay so that we could
      * serve an authentication request coming right after this one. */
-    if (m_applicationInfo.value("id").toString() !=
-        QStringLiteral("system-settings")) {
+    if (m_view->isVisible() &&
+        m_applicationInfo.value("id").toString() != "system-settings") {
         q->setDelay(3000);
     }
     QVariantMap result;
 
=== modified file 'online-accounts-ui/qml/ProviderRequest.qml'
--- online-accounts-ui/qml/ProviderRequest.qml	2016-05-19 11:06:28 +0000
+++ online-accounts-ui/qml/ProviderRequest.qml	2016-06-23 13:59:19 +0000
@@ -24,14 +24,10 @@
 MainView {
     id: root
 
-    property variant applicationInfo: application
-    property variant providerInfo: provider
-    property bool wasDenied: false
+    property var applicationInfo: request.application
+    property var providerInfo: request.provider
     property int __createdAccountId: 0
 
-    signal denied
-    signal allowed(int accountId)
-
     width: units.gu(48)
     height: units.gu(60)
     automaticOrientation: true
@@ -40,14 +36,14 @@
         i18n.domain = "ubuntu-system-settings-online-accounts"
         if (accessModel.count === 0 && !accessModel.canCreateAccounts) {
             /* No accounts to authorize */
-            denied()
+            request.deny()
             return
         }
-        if (!application.id && accessModel.count == 1 &&
+        if (!applicationInfo.id && accessModel.count == 1 &&
             applicationInfo.profile == "unconfined") {
             /* Degenerate case: unconfined app making requests with no valid
              * app ID */
-            allowed(accountsModel.get(0, "accountId"))
+            request.allow(accountsModel.get(0, "accountId"))
             return
         }
         loader.active = true
@@ -55,9 +51,6 @@
     }
 
     on__CreatedAccountIdChanged: grantAccessIfReady()
-    onDenied: wasDenied = true
-
-    onAllowed: loader.sourceComponent = spinnerComponent
 
     PageStack {
         id: pageStack
@@ -111,7 +104,7 @@
         AccountCreationPage {
             providerId: providerInfo.id
             onFinished: {
-                if (accountId == 0) root.denied()
+                if (accountId == 0) request.deny()
                 /* if an account was created, just remember its ID. when the
                  * accountsModel will notice it we'll proceed with the access
                  * grant */
@@ -127,7 +120,7 @@
             application: applicationInfo
             provider: providerInfo
             canAddAnotherAccount: accessModel.canCreateAccounts
-            onDenied: root.denied()
+            onDenied: request.deny()
             onAllowed: root.grantAccess(accountId)
             onCreateAccount: pageStack.push(createAccountPageComponent)
         }
@@ -198,7 +191,8 @@
         if (root.__createdAccountId != 0) {
             // If the request comes from system settings, stop here
             if (applicationInfo.id === "system-settings") {
-                root.allowed(root.__createdAccountId)
+                request.allow(root.__createdAccountId)
+                loader.sourceComponent = spinnerComponent
                 return
             }
 
@@ -216,7 +210,7 @@
         if (i < 0) {
             // very unlikely; maybe the account has been deleted in the meantime
             console.log("Account not found:" + accountId)
-            root.denied()
+            request.deny()
             return
         }
 
@@ -249,6 +243,7 @@
 
     function accountEnablingDone() {
         console.log("account enabling done")
-        allowed(account.accountId)
+        request.allow(account.accountId)
+        loader.sourceComponent = spinnerComponent
     }
 }
 
=== modified file 'tests/online-accounts-ui/tst_provider_request.cpp'
--- tests/online-accounts-ui/tst_provider_request.cpp	2015-06-23 13:17:10 +0000
+++ tests/online-accounts-ui/tst_provider_request.cpp	2016-06-23 13:59:19 +0000
@@ -173,12 +173,13 @@
         QTRY_COMPARE(setWindowCalled.count(), 1);
         QQuickView *view = static_cast<QQuickView*>(setWindowCalled.at(0).at(0).value<QWindow*>());
         QQmlContext *context = view->engine()->rootContext();
+        QObject *request = context->contextProperty("request").value<QObject*>();
 
         QCOMPARE(applicationInfoCalled.count(), 1);
         QCOMPARE(applicationInfoCalled.at(0).at(1).toString(), profile);
 
-        QCOMPARE(context->contextProperty("provider").toMap(), providerInfo);
-        QCOMPARE(context->contextProperty("application").toMap(), applicationInfo);
+        QCOMPARE(request->property("provider").toMap(), providerInfo);
+        QCOMPARE(request->property("application").toMap(), applicationInfo);
     } else {
         QTRY_COMPARE(failCalled.count(), 1);
         QCOMPARE(failCalled.at(0).at(0).toString(), errorName);