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

[Alberto Mardegan]

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.

[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.

[Alberto Mardegan (mardy)]

> Can you explain why security decisions shouldn't be made based on service ID? The entire security model of this simplified API is built around the idea that we _can_ use them as a basis for security decisions: confined apps can only make use of services that belong to their package, and unconfined apps can access everything (because they aren't being controlled by AppArmor).

No, it doesn't work like that. The decision is taken on the combination of account + service: as I wrote before, if your app uses the google account and ships a service file for it, it won't automatically be granted access to the account.

> In this model, there may be usable accounts that haven't been used with this service ID yet, but that's what Authenticate() is there for.

You mean RequestAccess, or whatever we'll call it?

> As for the rest, if you have specific filters in mind for this method, please make them real method arguments. We haven't set the D-Bus API in stone yet, so if there are important features that are missing we should just add them.

No, I really think this should be an a{sv}, because sometimes you use some filters (account Id), sometimes others (app ID).

> The details of OAuth client IDs or scopes are the realm of the service configuration file though, right? Can you point out cases where this feature is needed?

When the app developer wants to store them in the binary. This allows the same app to authenticate different times with different keys; for example, an app which is both an email and a calendar app, might want to use two separate client IDs (this covers the case that Thomas was talking about, I assume). Also, the scope list can be dynamic.

> In contrast, I don't know what "RequestAccess" sounds like it is used to gain access to something that already exists on the system.

The app doesn't know whether the account exists or not. What the API does is requesting OA to obtain a *new* (for the app) account. Maybe RequestAccount is a better name?

[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

=== modified file 'src/daemon/manager.cpp'
--- src/daemon/manager.cpp	2015-02-11 09:08:57 +0000
+++ src/daemon/manager.cpp	2015-02-18 12:26:47 +0000
@@ -82,34 +82,21 @@
     return hasAccess;
 }
 
-QList<AccountInfo> Manager::GetAccounts(const QStringList &serviceIds)
-{
-    Q_FOREACH(const QString &serviceId, serviceIds) {
-        if (!checkAccess(serviceId)) {
-            return QList<AccountInfo>();
-        }
-    }
-
-    return QList<AccountInfo>({AccountInfo(0, QVariantMap())});
-}
-
-AccountInfo Manager::GetAccountInfo(const QString &serviceId, uint accountId)
-{
-    Q_UNUSED(accountId);
-
-    if (!checkAccess(serviceId)) {
-        return AccountInfo();
-    }
-
-    return AccountInfo(accountId, QVariantMap());
-}
-
-QVariantMap Manager::Authenticate(const QString &serviceId, uint accountId,
-                                  bool interactive, bool invalidate)
+QList<AccountInfo> Manager::GetAccounts(const QVariantMap &filters)
+{
+    Q_UNUSED(filters);
+
+    return QList<AccountInfo>();
+}
+
+QVariantMap Manager::Authenticate(uint 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();
@@ -118,13 +105,16 @@
     return QVariantMap();
 }
 
-AccountInfo Manager::Register(const QString &serviceId, QVariantMap &credentials)
+AccountInfo Manager::RequestAccess(const QString &serviceId,
+                                   const QVariantMap &parameters,
+                                   QVariantMap &credentials)
 {
-    Q_UNUSED(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-11 12:59:23 +0000
+++ src/daemon/manager.h	2015-02-18 12:26:47 +0000
@@ -31,15 +31,16 @@
     ~Manager();
 
 public Q_SLOTS:
-    QList<AccountInfo> GetAccounts(const QStringList &serviceIds);
-    AccountInfo GetAccountInfo(const QString &serviceId, uint accountId);
-    QVariantMap Authenticate(const QString &serviceId, uint accountId,
-                             bool interactive, bool invalidate);
-    AccountInfo Register(const QString &serviceId, QVariantMap &credentials);
+    QList<AccountInfo> GetAccounts(const QVariantMap &filters);
+    QVariantMap Authenticate(uint 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 &serviceId, uint accountId, bool enabled);
-    void CredentialsChanged(const QString &serviceId, uint accountId);
+    void AccountChanged(const QString &serviceId, AccountInfo accountInfo);
 
 private:
     bool canAccess(const QString &serviceId);

=== modified file 'src/daemon/manager.xml'
--- src/daemon/manager.xml	2015-02-10 07:33:36 +0000
+++ src/daemon/manager.xml	2015-02-18 12:26:47 +0000
@@ -5,87 +5,148 @@
   <interface name="com.ubuntu.OnlineAccounts.Manager">
 
     <!--
-        GetAccounts: returns a list containing account information for
-        accounts that provide one of a list of service IDs.
+      Global concepts:
+
+      Clients are identified by their "applicationId", which is the
+      "<package>_<app>" of the client.
+
+      The "serviceId" is an application-specific name for an account provider.
+      That is, the "serviceId" contains the information about the application
+      which will use it, and the account provider (e.g. "google", "facebook").
+      "serviceId"s are backed by files shipped with the application, which can
+      contain application/account-provider specific information, such as the
+      ClientId and ClientSecret to be used when authentication with OAuth 2.0. 
+      An application can ship more than one "serviceId", if they refer to
+      different account providers; that is, one application can have a
+      serviceId for Google and one for Facebook, but the same application
+      cannot have one for GMail and another for Picasa.
+    -->
+
+    <!--
+      GetAccounts: returns a list of account IDs that satisfy the given
+      filters.
+
+      Allowed keys for the "filters" parameter (any combination is allowed,
+      though not all combinations might make sense):
+
+      - "applicationId" ("s"): the "<package>_<app>" of the client. An
+        unconfined application can specify this in order to restrict the set
+        of results to only those accounts that the application has been
+        authorized to use. For confined apps, OA will deduce the
+        applicationId from the apparmor label of the caller.
+
+      - "serviceId" ("s"): if the application wants to list only those
+        accounts coming from a specific provider (e.g., "Facebook").
+
+      - "accountId" ("u"): the ID of an account.
+
+      In any case, an application will receive only those accounts which the
+      user has authorized the application to use. See
+        http://wiki.ubuntu.com/OnlineAccounts
+      for an overview of the UI experience.
+
+      Disabled accounts will not be returned.
     -->
     <method name="GetAccounts">
-      <arg name="service_ids" type="as" direction="in" />
+      <arg name="filters" type="a{sv}" direction="in" />
+      <!--
+        The return value is a list of account IDs paired with a dictionary of account data.
+        This will always include:
+        - "serviceId" (s)
+        - "displayName" (s)
+        Other settings stored on the account will also be included, such as the
+        server address and port of an owncloud or IMAP account, but we haven't
+        defined exactly how. Possibly, all account-specific keys will be
+        prefixed by "settings/", in order not to clash with the keys defined at
+        the framework level.
+      -->
       <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.
-
-        If "interactive" is false, an error will be returned if
-        user interaction would be required to retrieve the
-        credentials.
-
-        If "invalidate" is true, any stored credentials will be
-        ignored and new credentials will be requested from the account
-        provider.
+      Authenticate: request authentication credentials for the given
+      account ID in the context of a particular service.
+
+      If "interactive" is false, an error will be returned if
+      user interaction would be required to retrieve the
+      credentials.
+
+      If "invalidate" is true, any stored credentials will be
+      ignored and new credentials will be requested from the account
+      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" /> -->
+      <!--
+        This dictionary can be used to specify OAuth client keys or permission
+        scopes. While it's possible to statically define them a the "serviceId"
+        level, some apps might want to change this information at runtime.
+        Possible use cases:
+        1) OAuth keys are retrieved from a server (so that they can be updated
+           when the old ones are revoked).
+        2) An app might want to use different keys when talking to GMail and
+           Picasa.
+        3) The scope list can also be dynamic, and depending on how the user is
+           using the app.
+        4) SASL: the parameters would contain the latest challenge got from the
+           server.
+      --> 
+      <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
-        service.
+      RequestAccess: register a new account for use with the given
+      service.
+      This method also performs the authentication, since that's what apps
+      would do as soon as they get the account.
     -->
-    <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().
+      AccountChanged: emitted when account details are changed.
+
+      The apparmor policy will NOT ALLOW confined applications to receive
+      this signal on the account manager object path (which might be
+      /com/ubuntu/OnlineAccounts/Manager). Only unconfined applications will
+      be able to listen to this signal from that object path.
+
+      Confined apps will have to catch this signal from a different object path:
+      /com/ubuntu/OnlineAccounts/Manager/@{APP_PKGNAME_DBUS}, which will
+      deliver the signal only for those accounts which the app has been
+      authorized to use.
     -->
     <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 serviceId will also be included in the "account" dictionary. The
+        reason for having it also here is to allow clients to filter D-Bus
+        messages by arg0. This can be important for unconfined clients, which
+        would otherwise be woken up by any account change, even those not
+          relevant to them.
+      -->
+      <arg name="serviceId" type="s" />
+      <!--
+        The dictionary contains a changeType key, type "u", whose value is
+        enum { enabled, disabled, changed }
+      -->
+      <arg name="account" type="(ua{sv})" />
+      <annotation name="org.qtproject.QtDBus.QtTypeName.In1" value="AccountInfo"/>
     </signal>
 
   </interface>