Merge lp:~mardy/online-accounts-api/dbus-api into lp:online-accounts-api

[Alberto Mardegan]

Update D-Bus API, fix warnings

Update D-Bus API, fix warnings

[James Henstridge (jamesh)]

Hi Alberto,

Many of these changes seem to be things that we disagreed about in the meeting previously. I have also been working on updating the interface as we discussed at that meeting, so can we hold off on this?

I've left some inline comments in the diff.

review: Needs Fixing

[Alberto Mardegan (mardy)]

> Many of these changes seem to be things that we disagreed about in the meeting
> previously. I have also been working on updating the interface as we
> discussed at that meeting, so can we hold off on this?

I'm proposing this exactly in order to discuss about the disagreements. :-)
Keep in mind that what we currently have in trunk has not been agreed upon either. Anyway, I won't merge this until we've got a consensus, or until we come to the conclusion that a consensus cannot be reached.
On the other hand, defining this interface is our top priority, because all the client side will be blocked until we settle this.

[lp:~mardy/online-accounts-api/dbus-api]

5. By Alberto Mardegan on 2015-02-10

Update

[Alberto Mardegan (mardy)]

I just pushed a new commit, taking your comments into account. I also replied to them, but in order to see my replies you'll have to select the "r4 into r3" diff into the "Preview Diff" selection box below.

[James Henstridge (jamesh)]

Replies to r4 comments.

review: Needs Fixing

[James Henstridge (jamesh)]

Now for r5:

As a general question, what style guide are you trying to convert this code to conform to exactly? I'd really appreciate it if you didn't mix these kinds of code clean ups in with functional changes: all it does is obscure what you're changing.

[lp:~mardy/online-accounts-api/dbus-api]

6. By Alberto Mardegan on 2015-02-11

merge from style

7. By Alberto Mardegan on 2015-02-11

Update

8. By Alberto Mardegan on 2015-02-11

style

9. By Alberto Mardegan on 2015-02-11

merge style

10. By Alberto Mardegan on 2015-02-18

from trunk

11. By Alberto Mardegan on 2015-02-18

Update docstring

Unmerged revisions

=== modified file 'src/daemon/aacontext.cpp'
--- src/daemon/aacontext.cpp	2015-02-10 08:12:59 +0000
+++ src/daemon/aacontext.cpp	2015-02-10 16:00:33 +0000
@@ -1,47 +1,52 @@
+#include <QDBusConnection>
+#include <QDBusMessage>
+#include <QDebug>
 #include "aacontext.h"
-#include <stdexcept>
 #include <sys/apparmor.h>
-#include <QDebug>
-#include <QDBusConnection>
-#include <QDBusMessage>
-
-const int max_cache_size = 50;
-
-AppArmorContext::AppArmorContext() {
-}
-
-AppArmorContext::~AppArmorContext() {
-}
-
-QString AppArmorContext::getPeerSecurityContext(const QDBusConnection &bus, const QDBusMessage &message) {
+
+const int maxCacheSize = 50;
+
+AppArmorContext::AppArmorContext()
+{
+}
+
+AppArmorContext::~AppArmorContext()
+{
+}
+
+QString AppArmorContext::getPeerSecurityContext(const QDBusConnection &bus,
+                                                const QDBusMessage &message)
+{
     if (!aa_is_enabled()) {
         return "unconfined";
     }
 
     QString peer_address = message.service();
-    try {
-        return contexts.at(peer_address);
-    } catch (const std::out_of_range &e) {
+    QString &context = m_contexts[peer_address];
+    /* If the peer_address was now known, it's now been added as an empty
+     * QString. */
+    if (!context.isEmpty()) {
+        return context;
     }
 
-    QDBusMessage msg = QDBusMessage::createMethodCall(
-        "org.freedesktop.DBus", "/org/freedesktop/DBus",
-        "org.freedesktop.DBus", "GetConnectionAppArmorSecurityContext");
+    QDBusMessage msg =
+        QDBusMessage::createMethodCall("org.freedesktop.DBus",
+                                       "/org/freedesktop/DBus",
+                                       "org.freedesktop.DBus",
+                                       "GetConnectionAppArmorSecurityContext");
     msg << peer_address;
     QDBusMessage reply = bus.call(msg, QDBus::Block);
 
-    QString context;
     if (reply.type() == QDBusMessage::ReplyMessage) {
         context = reply.arguments().value(0).value<QString>();
     } else {
-        qWarning() << "Could not determine AppArmor context: "
-                   << reply.errorName() << ": " << reply.errorMessage();
+        qWarning() << "Could not determine AppArmor context: " <<
+            reply.errorName() << ": " << reply.errorMessage();
     }
 
     // If the context cache has hit the maximum size, clear it
-    if (contexts.size() >= max_cache_size) {
-        contexts.clear();
+    if (m_contexts.size() >= maxCacheSize) {
+        m_contexts.clear();
     }
-    contexts[peer_address] = context;
     return context;
 }

=== modified file 'src/daemon/aacontext.h'
--- src/daemon/aacontext.h	2015-02-09 05:18:22 +0000
+++ src/daemon/aacontext.h	2015-02-10 16:00:33 +0000
@@ -1,9 +1,9 @@
 #ifndef AACONTEXT_H
 #define AACONTEXT_H
 
-#include <map>
 #include <QDBusConnection>
 #include <QDBusMessage>
+#include <QHash>
 #include <QString>
 
 class AppArmorContext {
@@ -14,7 +14,7 @@
     QString getPeerSecurityContext(const QDBusConnection &bus, const QDBusMessage &message);
 
 private:
-    std::map<QString,QString> contexts;
+    QHash<QString,QString> m_contexts;
 };
 
 #endif

=== modified file 'src/daemon/main.cpp'
--- src/daemon/main.cpp	2015-02-10 06:24:38 +0000
+++ src/daemon/main.cpp	2015-02-10 16:00:33 +0000
@@ -1,7 +1,8 @@
 #include "manager.h"
 #include "manageradaptor.h"
 
-int main(int argc, char **argv) {
+int main(int argc, char **argv)
+{
     QCoreApplication app(argc, argv);
 
     qDBusRegisterMetaType<AccountInfo>();

=== modified file 'src/daemon/manager.cpp'
--- src/daemon/manager.cpp	2015-02-10 06:24:38 +0000
+++ src/daemon/manager.cpp	2015-02-10 16:00:33 +0000
@@ -1,40 +1,48 @@
-#include "manager.h"
+#include <QDBusMessage>
 #include <QDebug>
-#include <QDBusMessage>
 #include "aacontext.h"
-
-using namespace std;
+#include "manager.h"
 
 static const char FORBIDDEN_ERROR[] = "com.ubuntu.OnlineAccounts.Error.Forbidden";
 
-QDBusArgument &operator<<(QDBusArgument &argument, const AccountInfo &info) {
-    argument.beginStructure();
-    argument << info.account_id << info.details;
-    argument.endStructure();
-    return argument;
-}
-
-const QDBusArgument &operator>>(const QDBusArgument &argument, AccountInfo &info) {
-    argument.beginStructure();
-    argument >> info.account_id >> info.details;
-    argument.endStructure();
-    return argument;
-}
-
-
-struct Manager::Private {
-    AppArmorContext apparmor;
+QDBusArgument &operator<<(QDBusArgument &argument, const AccountInfo &info)
+{
+    argument.beginStructure();
+    argument << info.accountId << info.details;
+    argument.endStructure();
+    return argument;
+}
+
+const QDBusArgument &operator>>(const QDBusArgument &argument,
+                                AccountInfo &info)
+{
+    argument.beginStructure();
+    argument >> info.accountId >> info.details;
+    argument.endStructure();
+    return argument;
+}
+
+class ManagerPrivate {
+public:
+    AppArmorContext m_apparmor;
 };
 
-Manager::Manager(QObject *parent)
-    : QObject(parent), p(new Private) {
-}
-
-Manager::~Manager() {
-}
-
-bool Manager::canAccess(const QString &service_id) {
-    QString context = p->apparmor.getPeerSecurityContext(connection(), message());
+Manager::Manager(QObject *parent):
+    QObject(parent),
+    d_ptr(new ManagerPrivate)
+{
+}
+
+Manager::~Manager()
+{
+}
+
+bool Manager::canAccess(const QString &serviceId)
+{
+    Q_D(Manager);
+
+    QString context = d->m_apparmor.getPeerSecurityContext(connection(),
+                                                           message());
     // Could not determine peer's AppArmor context, so deny access
     if (context.isEmpty()) {
         return false;
@@ -55,51 +63,56 @@
     // Do the same on the service ID: we are only dealing with
     // confined apps at this point, so only $pkgname prefixed
     // services are accessible.
-    pos = service_id.indexOf('_');
+    pos = serviceId.indexOf('_');
     if (pos < 0) {
         return false;
     }
-    return service_id.left(pos) == pkgname;
-}
-
-bool Manager::checkAccess(const QString &service_id) {
-    bool has_access = canAccess(service_id);
-    if (!has_access) {
-        sendErrorReply(FORBIDDEN_ERROR, QString("Access to service ID %1 forbidden").arg(service_id));
-    }
-    return has_access;
-}
-
-QList<AccountInfo> Manager::GetAccounts(const QStringList &service_ids) {
-    for (const auto &service_id : service_ids) {
-        if (!checkAccess(service_id)) {
-            return QList<AccountInfo>();
-        }
-    }
-
-    return QList<AccountInfo>({AccountInfo(0, QVariantMap())});
-}
-
-AccountInfo Manager::GetAccountInfo(const QString &service_id, uint account_id) {
-    if (!checkAccess(service_id)) {
-        return AccountInfo();
-    }
-
-    return AccountInfo(account_id, QVariantMap());
-}
-
-QVariantMap Manager::Authenticate(const QString &service_id, uint account_id, bool interactive, bool invalidate) {
-    if (!checkAccess(service_id)) {
+    return serviceId.left(pos) == pkgname;
+}
+
+bool Manager::checkAccess(const QString &serviceId)
+{
+    bool hasAccess = canAccess(serviceId);
+    if (!hasAccess) {
+        sendErrorReply(FORBIDDEN_ERROR,
+                       QString("Access to service ID %1 forbidden").arg(serviceId));
+    }
+    return hasAccess;
+}
+
+QList<AccountInfo> Manager::GetAccounts(const QVariantMap &filters)
+{
+    Q_UNUSED(filters);
+
+    return QList<AccountInfo>();
+}
+
+QVariantMap Manager::Authenticate(quint32 accountId, const QString &serviceId,
+                                  bool interactive, bool invalidate,
+                                  const QVariantMap &parameters)
+{
+    Q_UNUSED(accountId);
+    Q_UNUSED(interactive);
+    Q_UNUSED(invalidate);
+    Q_UNUSED(parameters);
+
+    if (!checkAccess(serviceId)) {
         return QVariantMap();
     }
 
     return QVariantMap();
 }
 
-AccountInfo Manager::Register(const QString &service_id, QVariantMap &credentials) {
-    if (!checkAccess(service_id)) {
+AccountInfo Manager::RequestAccess(const QString &serviceId,
+                                   const QVariantMap &parameters,
+                                   QVariantMap &credentials)
+{
+    Q_UNUSED(parameters);
+
+    if (!checkAccess(serviceId)) {
         return AccountInfo();
     }
 
+    credentials = QVariantMap();
     return AccountInfo();
 }

=== modified file 'src/daemon/manager.h'
--- src/daemon/manager.h	2015-02-10 06:24:38 +0000
+++ src/daemon/manager.h	2015-02-10 16:00:33 +0000
@@ -1,50 +1,54 @@
-
 #ifndef MANAGER_H
 #define MANAGER_H
 
-#include <memory>
+#include <QDBusArgument>
+#include <QDBusContext>
 #include <QList>
 #include <QObject>
 #include <QVariantMap>
-#include <QDBusArgument>
-#include <QDBusContext>
 
 struct AccountInfo {
-    uint account_id = 0;
+    quint32 accountId;
     QVariantMap details;
 
-    AccountInfo() {}
-    AccountInfo(uint account_id, const QVariantMap &details)
-        : account_id(account_id), details(details) {}
-
+    AccountInfo(): accountId(0) {}
+    AccountInfo(uint accountId, const QVariantMap &details):
+        accountId(accountId), details(details) {}
 };
 Q_DECLARE_METATYPE(AccountInfo)
 
 QDBusArgument &operator<<(QDBusArgument &argument, const AccountInfo &info);
-const QDBusArgument &operator>>(const QDBusArgument &argument, AccountInfo &info);
+const QDBusArgument &operator>>(const QDBusArgument &argument,
+                                AccountInfo &info);
 
-class Manager : public QObject, protected QDBusContext {
+class ManagerPrivate;
+class Manager: public QObject, protected QDBusContext
+{
     Q_OBJECT
-    struct Private;
+
 public:
-    explicit Manager(QObject *parent=nullptr);
+    explicit Manager(QObject *parent = 0);
     ~Manager();
 
 public Q_SLOTS:
-    QList<AccountInfo> GetAccounts(const QStringList &service_ids);
-    AccountInfo GetAccountInfo(const QString &service_id, uint account_id);
-    QVariantMap Authenticate(const QString &service_id, uint account_id, bool interactive, bool invalidate);
-    AccountInfo Register(const QString &service_id, QVariantMap &credentials);
+    QList<AccountInfo> GetAccounts(const QVariantMap &filters);
+    QVariantMap Authenticate(quint32 accountId, const QString &serviceId,
+                             bool interactive, bool invalidate,
+                             const QVariantMap &parameters);
+    AccountInfo RequestAccess(const QString &serviceId,
+                              const QVariantMap &parameters,
+                              QVariantMap &credentials);
 
 Q_SIGNALS:
-    void AccountChanged(const QString &service_id, uint account_id, bool enabled);
-    void CredentialsChanged(const QString &service_id, uint account_id);
+    void AccountChanged(AccountInfo accountInfo);
 
 private:
-    bool canAccess(const QString &service_id);
-    bool checkAccess(const QString &service_id);
+    bool canAccess(const QString &serviceId);
+    bool checkAccess(const QString &serviceId);
     QString getPeerSecurityContext();
-    std::unique_ptr<Private> p;
+
+    Q_DECLARE_PRIVATE(Manager)
+    ManagerPrivate *d_ptr;
 };
 
 #endif

=== modified file 'src/daemon/manager.xml'
--- src/daemon/manager.xml	2015-02-10 07:33:36 +0000
+++ src/daemon/manager.xml	2015-02-10 16:00:33 +0000
@@ -5,27 +5,18 @@
   <interface name="com.ubuntu.OnlineAccounts.Manager">
 
     <!--
-        GetAccounts: returns a list containing account information for
-        accounts that provide one of a list of service IDs.
+        GetAccounts: returns a list of account IDs that satisfy the given
+        filters.
     -->
     <method name="GetAccounts">
-      <arg name="service_ids" type="as" direction="in" />
+      <arg name="filters" type="a{sv}" direction="in" />
       <arg name="accounts" type="a(ua{sv})" direction="out" />
+      <annotation name="org.qtproject.QtDBus.QtTypeName.In0" value="QVariantMap"/>
       <annotation name="org.qtproject.QtDBus.QtTypeName.Out0"
                   value="QList&lt;AccountInfo&gt;"/>
     </method>
 
     <!--
-        GetAccountInfo: return information about a given account ID
-    -->
-    <method name="GetAccountInfo">
-      <arg name="service_id" type="s" direction="in" />
-      <arg name="account_id" type="u" direction="in" />
-      <arg name="details" type="(ua{sv})" direction="out" />
-      <annotation name="org.qtproject.QtDBus.QtTypeName.Out0" value="AccountInfo"/>
-    </method>
-
-    <!--
         Authenticate: request authentication credentials for the given
         account ID in the context of a particualr service.
 
@@ -38,54 +29,39 @@
         provider.
     -->
     <method name="Authenticate">
-      <arg name="service_id" type="s" direction="in" />
-      <arg name="account_id" type="u" direction="in" />
+      <arg name="accountId" type="u" direction="in" />
+      <arg name="serviceId" type="s" direction="in" />
       <arg name="interactive" type="b" direction="in" />
       <arg name="invalidate" type="b" direction="in" />
-      <!-- <arg name="reply_socket" type="h" direction="in" /> -->
+      <arg name="parameters" type="a{sv}" direction="in" />
       <arg name="credentials" type="a{sv}" direction="out" />
+      <annotation name="org.qtproject.QtDBus.QtTypeName.In4" value="QVariantMap"/>
       <annotation name="org.qtproject.QtDBus.QtTypeName.Out0" value="QVariantMap"/>
     </method>
 
     <!--
-        Register: register a new account for use with the given
+        RequestAccess: register a new account for use with the given
         service.
     -->
-    <method name="Register">
-      <arg name="service_id" type="s" direction="in" />
-      <!-- <arg name="reply_socket" type="h" direction="in" /> -->
+    <method name="RequestAccess">
+      <arg name="serviceId" type="s" direction="in" />
+      <arg name="parameters" type="a{sv}" direction="in" />
       <arg name="account" type="(ua{sv})" direction="out" />
+      <arg name="credentials" type="a{sv}" direction="out" />
+      <annotation name="org.qtproject.QtDBus.QtTypeName.In1" value="QVariantMap"/>
       <annotation name="org.qtproject.QtDBus.QtTypeName.Out0" value="AccountInfo"/>
-      <arg name="credentials" type="a{sv}" direction="out" />
       <annotation name="org.qtproject.QtDBus.QtTypeName.Out1" value="QVariantMap"/>
     </method>
 
     <!--
         AccountChanged: emitted when account details are changed.
-
-        This signal will be emitted when new accounts are enabled, and
-        when existing accounts are disabled.  Clients can detect these
-        cases by checking the "enabled" flag and comparing the account
-        ID with the list of accounts they currently know about.
-
-        The actual changed account details can be retrieved with
-        GetAccountInfo().
     -->
     <signal name="AccountChanged">
-      <arg name="service_id" type="s" />
-      <arg name="account_id" type="u" />
-      <arg name="enabled" type="b" />
-    </signal>
-
-    <!--
-        CredentialsChanged: emitted when the credentials for the given
-        service ID on the given account change.
-
-        The new credentials can be retrieved with Authenticate().
-    -->
-    <signal name="CredentialsChanged">
-      <arg name="service_id" type="s" />
-      <arg name="account_id" type="u" />
+      <!-- the dictionary  ontains a changeType key, type "u", whose value is
+          enum { enabled, disabled, changed }
+      -->
+      <arg name="account" type="(ua{sv})" />
+      <annotation name="org.qtproject.QtDBus.QtTypeName.In0" value="AccountInfo"/>
     </signal>
 
   </interface>