Merge lp:~mardy/online-accounts-api/password-1628473 into lp:online-accounts-api

[Alberto Mardegan]

Fix reply of password-based authentication

The keys for the username and password are different from those used by signond, and need to be translated.

Fix reply of password-based authentication

The keys for the username and password are different from those used by signond, and need to be translated.

[lp:~mardy/online-accounts-api/password-1628473]

28. By Alberto Mardegan on 2016-10-06

From trunk

* debian/control:
  - Depend on qttools5-dev-tools for qdoc (LP: #1608814)
* debian/rules:
  - No need to invoke dh_python

[David Barth (dbarth)]

LGTM. Nice code reorg as well.

review: Approve

=== modified file 'src/lib/OnlineAccountsDaemon/authenticator.cpp'
--- src/lib/OnlineAccountsDaemon/authenticator.cpp	2016-02-12 07:44:03 +0000
+++ src/lib/OnlineAccountsDaemon/authenticator.cpp	2016-10-06 06:56:41 +0000
@@ -71,6 +71,7 @@
     SignOn::AuthSession *m_authSession;
     SignOn::Identity *m_identity;
     QVariantMap m_parameters;
+    int m_authMethod;
     QVariantMap m_reply;
     QVariantMap m_extraReplyData;
     QString m_errorName;
@@ -85,6 +86,7 @@
     QObject(q),
     m_authSession(0),
     m_identity(0),
+    m_authMethod(ONLINE_ACCOUNTS_AUTH_METHOD_UNKNOWN),
     m_invalidateCache(false),
     q_ptr(q)
 {
@@ -111,11 +113,14 @@
         mergeMaps(authData.parameters(), mergeMaps(parameters, m_parameters));
     QString mechanism = authData.mechanism();
 
+    m_authMethod = Authenticator::authMethod(authData);
+
     if (m_invalidateCache) {
         /* This works for OAuth 1.0 and 2.0; other authentication plugins should
          * implement a similar flag. */
         allSessionData["ForceTokenRefresh"] = true;
-        if (authData.method() == "password" || authData.method() == "sasl") {
+        if (m_authMethod == ONLINE_ACCOUNTS_AUTH_METHOD_PASSWORD ||
+            m_authMethod == ONLINE_ACCOUNTS_AUTH_METHOD_SASL) {
             uint uiPolicy = allSessionData.value("UiPolicy").toUInt();
             if (uiPolicy != SignOn::NoUserInteractionPolicy) {
                 allSessionData["UiPolicy"] = SignOn::RequestPasswordPolicy;
@@ -124,7 +129,7 @@
     }
 
     m_extraReplyData.clear();
-    if (mechanism == "HMAC-SHA1" || mechanism == "PLAINTEXT") {
+    if (m_authMethod == ONLINE_ACCOUNTS_AUTH_METHOD_OAUTH1) {
         /* For OAuth 1.0, let's return also the Consumer key and secret along
          * with the reply. */
         m_extraReplyData[ONLINE_ACCOUNTS_AUTH_KEY_CONSUMER_KEY] =
@@ -139,7 +144,17 @@
 void AuthenticatorPrivate::onAuthSessionResponse(const SignOn::SessionData &sessionData)
 {
     Q_Q(Authenticator);
-    m_reply = mergeMaps(m_extraReplyData, sessionData.toMap());
+    QVariantMap signonReply;
+
+    /* Perform some method-specific translation of reply keys */
+    if (m_authMethod == ONLINE_ACCOUNTS_AUTH_METHOD_PASSWORD) {
+        signonReply[ONLINE_ACCOUNTS_AUTH_KEY_USERNAME] = sessionData.UserName();
+        signonReply[ONLINE_ACCOUNTS_AUTH_KEY_PASSWORD] = sessionData.Secret();
+    } else {
+        signonReply = sessionData.toMap();
+    }
+
+    m_reply = mergeMaps(m_extraReplyData, signonReply);
     Q_EMIT q->finished();
 }
 
@@ -231,4 +246,23 @@
     return d->m_errorMessage;
 }
 
+int Authenticator::authMethod(const Accounts::AuthData &authData)
+{
+    QString method = authData.method();
+    QString mechanism = authData.mechanism();
+    if (method == "oauth2") {
+        if (mechanism == "web_server" || mechanism == "user_agent") {
+            return ONLINE_ACCOUNTS_AUTH_METHOD_OAUTH2;
+        } else if (mechanism == "HMAC-SHA1" || mechanism == "PLAINTEXT") {
+            return ONLINE_ACCOUNTS_AUTH_METHOD_OAUTH1;
+        }
+    } else if (method == "sasl") {
+        return ONLINE_ACCOUNTS_AUTH_METHOD_SASL;
+    } else if (method == "password") {
+        return ONLINE_ACCOUNTS_AUTH_METHOD_PASSWORD;
+    }
+
+    return ONLINE_ACCOUNTS_AUTH_METHOD_UNKNOWN;
+}
+
 #include "authenticator.moc"
 
=== modified file 'src/lib/OnlineAccountsDaemon/authenticator.h'
--- src/lib/OnlineAccountsDaemon/authenticator.h	2015-08-28 14:25:23 +0000
+++ src/lib/OnlineAccountsDaemon/authenticator.h	2016-10-06 06:56:41 +0000
@@ -51,6 +51,8 @@
     QString errorName() const;
     QString errorMessage() const;
 
+    static int authMethod(const Accounts::AuthData &authData);
+
 Q_SIGNALS:
     void finished();
 
 
=== modified file 'src/lib/OnlineAccountsDaemon/manager.cpp'
--- src/lib/OnlineAccountsDaemon/manager.cpp	2016-01-13 14:23:12 +0000
+++ src/lib/OnlineAccountsDaemon/manager.cpp	2016-10-06 06:56:41 +0000
@@ -33,6 +33,7 @@
 #include <QSet>
 #include "access_request.h"
 #include "authentication_request.h"
+#include "authenticator.h"
 #include "client_registry.h"
 #include "dbus_constants.h"
 #include "manager_adaptor.h"
@@ -76,7 +77,6 @@
                                     const QString &serviceName,
                                     const QStringList &clients);
 
-    int authMethod(const Accounts::AuthData &authData);
     AccountInfo readAccountInfo(const Accounts::AccountService *as);
     QList<AccountInfo> getAccounts(const QVariantMap &filters,
                                    const CallContext &context);
@@ -317,25 +317,6 @@
     return activeAccount;
 }
 
-int ManagerPrivate::authMethod(const Accounts::AuthData &authData)
-{
-    QString method = authData.method();
-    QString mechanism = authData.mechanism();
-    if (method == "oauth2") {
-        if (mechanism == "web_server" || mechanism == "user_agent") {
-            return ONLINE_ACCOUNTS_AUTH_METHOD_OAUTH2;
-        } else if (mechanism == "HMAC-SHA1" || mechanism == "PLAINTEXT") {
-            return ONLINE_ACCOUNTS_AUTH_METHOD_OAUTH1;
-        }
-    } else if (method == "sasl") {
-        return ONLINE_ACCOUNTS_AUTH_METHOD_SASL;
-    } else if (method == "password") {
-        return ONLINE_ACCOUNTS_AUTH_METHOD_PASSWORD;
-    }
-
-    return ONLINE_ACCOUNTS_AUTH_METHOD_UNKNOWN;
-}
-
 AccountInfo ManagerPrivate::readAccountInfo(const Accounts::AccountService *as)
 {
     QVariantMap info;
@@ -343,7 +324,7 @@
     info[ONLINE_ACCOUNTS_INFO_KEY_DISPLAY_NAME] = as->account()->displayName();
     info[ONLINE_ACCOUNTS_INFO_KEY_SERVICE_ID] = as->service().name();
 
-    info[ONLINE_ACCOUNTS_INFO_KEY_AUTH_METHOD] = authMethod(as->authData());
+    info[ONLINE_ACCOUNTS_INFO_KEY_AUTH_METHOD] = Authenticator::authMethod(as->authData());
     QString settingsPrefix(QStringLiteral(ONLINE_ACCOUNTS_INFO_KEY_SETTINGS));
     /* First, read the global settings */
     Accounts::Account *a = as->account();
 
=== modified file 'tests/daemon/functional_tests/data/com.ubuntu.tests_coolshare.service'
--- tests/daemon/functional_tests/data/com.ubuntu.tests_coolshare.service	2015-07-08 15:09:56 +0000
+++ tests/daemon/functional_tests/data/com.ubuntu.tests_coolshare.service	2016-10-06 06:56:41 +0000
@@ -4,4 +4,11 @@
   <name>Cool Share</name>
   <icon>general_otherservice</icon>
   <provider>cool</provider>
+
+  <template>
+    <group name="auth">
+      <setting name="method">password</setting>
+      <setting name="mechanism">password</setting>
+    </group>
+  </template>
 </service>
 
=== modified file 'tests/daemon/functional_tests/fake_signond.h'
--- tests/daemon/functional_tests/fake_signond.h	2015-07-06 14:51:30 +0000
+++ tests/daemon/functional_tests/fake_signond.h	2016-10-06 06:56:41 +0000
@@ -37,6 +37,10 @@
         mockedAuthService().call("AddIdentity", id, info);
     }
 
+    void setNextReply(uint identity, const QVariantMap &reply) {
+        mockedAuthService().call("SetNextReply", identity, reply);
+    }
+
 private:
     OrgFreedesktopDBusMockInterface &mockedAuthService() {
         return m_mock->mockInterface("com.google.code.AccountsSSO.SingleSignOn",
 
=== modified file 'tests/daemon/functional_tests/functional_tests.cpp'
--- tests/daemon/functional_tests/functional_tests.cpp	2016-07-22 05:08:11 +0000
+++ tests/daemon/functional_tests/functional_tests.cpp	2016-10-06 06:56:41 +0000
@@ -186,6 +186,7 @@
     EnvSetup m_env;
     DBusService *m_dbus;
     int m_firstAccountId;
+    int m_account2CredentialsId;
     int m_account3CredentialsId;
 };
 
@@ -206,6 +207,7 @@
 FunctionalTests::FunctionalTests():
     QObject(),
     m_dbus(0),
+    m_account2CredentialsId(41),
     m_account3CredentialsId(35)
 {
     clearDb();
@@ -230,6 +232,7 @@
     QVERIFY(account2 != 0);
     account2->setEnabled(true);
     account2->setDisplayName("CoolAccount 2");
+    account2->setCredentialsId(m_account2CredentialsId);
     account2->selectService(coolMail);
     account2->setEnabled(false);
     account2->selectService(coolShare);
@@ -262,6 +265,9 @@
 {
     m_dbus = new DBusService();
     m_dbus->startServices();
+
+    /* Uncomment next line to debug DBus calls */
+    // QProcess::startDetached("/usr/bin/dbus-monitor");
 }
 
 void FunctionalTests::cleanup()
@@ -321,16 +327,19 @@
     QTest::addColumn<bool>("interactive");
     QTest::addColumn<bool>("invalidate");
     QTest::addColumn<QVariantMap>("authParams");
+    QTest::addColumn<QVariantMap>("signonReply");
     QTest::addColumn<QVariantMap>("expectedCredentials");
     QTest::addColumn<QString>("errorName");
 
     QVariantMap authParams;
+    QVariantMap signonReply;
     QVariantMap credentials;
     QTest::newRow("invalid account ID") <<
         12412341 <<
         "coolmail" <<
         false << false <<
         authParams <<
+        signonReply <<
         credentials <<
         ONLINE_ACCOUNTS_ERROR_PERMISSION_DENIED;
 
@@ -341,6 +350,7 @@
         "coolmail" <<
         false << false <<
         authParams <<
+        signonReply <<
         credentials <<
         "com.ubuntu.OnlineAccounts.Error.Network";
     authParams.clear();
@@ -354,6 +364,7 @@
         "coolmail" <<
         false << false <<
         authParams <<
+        signonReply <<
         credentials <<
         QString();
 
@@ -363,6 +374,7 @@
         "coolmail" <<
         true << false <<
         authParams <<
+        signonReply <<
         credentials <<
         QString();
 
@@ -372,6 +384,7 @@
         "coolmail" <<
         true << true <<
         authParams <<
+        signonReply <<
         credentials <<
         QString();
 
@@ -385,6 +398,7 @@
         "oauth1auth" <<
         true << false <<
         authParams <<
+        signonReply <<
         credentials <<
         QString();
 
@@ -399,6 +413,25 @@
         "oauth1auth" <<
         true << false <<
         authParams <<
+        signonReply <<
+        credentials <<
+        QString();
+
+    authParams.clear();
+    credentials = QVariantMap {
+        { ONLINE_ACCOUNTS_AUTH_KEY_USERNAME, "my name" },
+        { ONLINE_ACCOUNTS_AUTH_KEY_PASSWORD, "don't tell" },
+    };
+    signonReply = QVariantMap {
+        { "UserName", "my name" },
+        { "Secret", "don't tell" },
+    };
+    QTest::newRow("password") <<
+        2 <<
+        "com.ubuntu.tests_coolshare" <<
+        true << false <<
+        authParams <<
+        signonReply <<
         credentials <<
         QString();
 }
@@ -410,10 +443,16 @@
     QFETCH(bool, interactive);
     QFETCH(bool, invalidate);
     QFETCH(QVariantMap, authParams);
+    QFETCH(QVariantMap, signonReply);
     QFETCH(QVariantMap, expectedCredentials);
     QFETCH(QString, errorName);
 
+    m_dbus->signond().addIdentity(m_account2CredentialsId, QVariantMap());
     m_dbus->signond().addIdentity(m_account3CredentialsId, QVariantMap());
+    if (!signonReply.isEmpty()) {
+        m_dbus->signond().setNextReply(m_account2CredentialsId, signonReply);
+        m_dbus->signond().setNextReply(m_account3CredentialsId, signonReply);
+    }
 
     DaemonInterface *daemon = new DaemonInterface(m_dbus->sessionConnection());
 
@@ -425,8 +464,10 @@
     if (errorName.isEmpty()) {
         QVERIFY2(!reply.isError(), reply.error().message().toUtf8().constData());
         QVariantMap credentials = reply.argumentAt<0>();
-        // Add the requestor PID
-        expectedCredentials["requestorPid"] = getpid();
+        if (credentials.contains("requestorPid")) {
+            // Add the requestor PID
+            expectedCredentials["requestorPid"] = getpid();
+        }
         QCOMPARE(credentials, expectedCredentials);
     } else {
         QVERIFY(reply.isError());
@@ -561,11 +602,14 @@
 
     QCOMPARE(serviceId, coolShare.name());
     QCOMPARE(accountInfo.id(), account->id());
-    QVariantMap expectedAccountInfo;
-    expectedAccountInfo["authMethod"] = ONLINE_ACCOUNTS_AUTH_METHOD_UNKNOWN;
-    expectedAccountInfo["changeType"] = ONLINE_ACCOUNTS_INFO_CHANGE_ENABLED;
-    expectedAccountInfo["displayName"] = "New account";
-    expectedAccountInfo["serviceId"] = "com.ubuntu.tests_coolshare";
+    QVariantMap expectedAccountInfo {
+        { "authMethod", ONLINE_ACCOUNTS_AUTH_METHOD_PASSWORD },
+        { "changeType", ONLINE_ACCOUNTS_INFO_CHANGE_ENABLED },
+        { "displayName", "New account" },
+        { "serviceId", "com.ubuntu.tests_coolshare" },
+        { "settings/auth/mechanism", "password" },
+        { "settings/auth/method", "password" },
+    };
     QCOMPARE(accountInfo.data(), expectedAccountInfo);
 
     /* Change a setting */
@@ -579,11 +623,15 @@
 
     QCOMPARE(serviceId, coolShare.name());
     QCOMPARE(accountInfo.id(), account->id());
-    expectedAccountInfo["authMethod"] = ONLINE_ACCOUNTS_AUTH_METHOD_UNKNOWN;
-    expectedAccountInfo["changeType"] = ONLINE_ACCOUNTS_INFO_CHANGE_UPDATED;
-    expectedAccountInfo["settings/color"] = "blue";
-    expectedAccountInfo["displayName"] = "New account";
-    expectedAccountInfo["serviceId"] = "com.ubuntu.tests_coolshare";
+    expectedAccountInfo = QVariantMap {
+        { "authMethod", ONLINE_ACCOUNTS_AUTH_METHOD_PASSWORD },
+        { "changeType", ONLINE_ACCOUNTS_INFO_CHANGE_UPDATED },
+        { "settings/color", "blue" },
+        { "displayName", "New account" },
+        { "serviceId", "com.ubuntu.tests_coolshare" },
+        { "settings/auth/mechanism", "password" },
+        { "settings/auth/method", "password" },
+    };
     QCOMPARE(accountInfo.data(), expectedAccountInfo);
 
     /* Delete the account */
@@ -597,10 +645,15 @@
 
     QCOMPARE(serviceId, coolShare.name());
     QCOMPARE(accountInfo.id(), account->id());
-    expectedAccountInfo["authMethod"] = ONLINE_ACCOUNTS_AUTH_METHOD_UNKNOWN;
-    expectedAccountInfo["changeType"] = ONLINE_ACCOUNTS_INFO_CHANGE_DISABLED;
-    expectedAccountInfo["displayName"] = "New account";
-    expectedAccountInfo["serviceId"] = "com.ubuntu.tests_coolshare";
+    expectedAccountInfo = QVariantMap {
+        { "authMethod", ONLINE_ACCOUNTS_AUTH_METHOD_PASSWORD },
+        { "changeType", ONLINE_ACCOUNTS_INFO_CHANGE_DISABLED },
+        { "displayName", "New account" },
+        { "serviceId", "com.ubuntu.tests_coolshare" },
+        { "settings/color", "blue" },
+        { "settings/auth/mechanism", "password" },
+        { "settings/auth/method", "password" },
+    };
     QCOMPARE(accountInfo.data(), expectedAccountInfo);
 
     delete manager;
 
=== modified file 'tests/daemon/functional_tests/signond.py'
--- tests/daemon/functional_tests/signond.py	2015-07-23 13:57:48 +0000
+++ tests/daemon/functional_tests/signond.py	2016-10-06 06:56:41 +0000
@@ -48,7 +48,11 @@
     return (path, self.identities[identity])
 
 
-def auth_session_process(identity, params, method):
+def auth_session_process(self, params, method):
+    auth_service = dbusmock.get_object(MAIN_OBJ)
+    if self.identity in auth_service.auth_replies:
+        return auth_service.auth_replies[self.identity]
+
     if 'errorName' in params:
         raise dbus.exceptions.DBusException('Authentication error',
                                             name=params['errorName'])
@@ -63,7 +67,7 @@
     path = '/AuthSession%s' % self.sessions_counter
     self.sessions_counter += 1
     self.AddObject(path, AUTH_SESSION_IFACE, {}, [
-        ('process', 'a{sv}s', 'a{sv}', 'ret = self.auth_session_process(self.identity, args[0], args[1])'),
+        ('process', 'a{sv}s', 'a{sv}', 'ret = self.auth_session_process(self, args[0], args[1])'),
     ])
 
     auth_session = dbusmock.get_object(path)
@@ -91,3 +95,7 @@
 def AddIdentity(self, identity, data):
     self.identities[identity] = data
 
[e-mail](MOCK_IFACE, in_signature='ua{sv}', out_signature='')
+def SetNextReply(self, identity, reply):
+    self.auth_replies[identity] = reply
+