Manager class only provides access to one (random?) service ID for an account

[James Henstridge]

The Manager class seems to be de-duplicating the list of accounts using accountId as a key. If an account has multiple service IDs, it seems like it is random chance whether I'll see the service ID I'm interested in.

As a test, I wrote a little program to list available accounts using Manager::availableAccounts(), which showed my Twitter account once:

    Account 7:
      Name: <email address hidden>
      Service ID: twitter-poll

But when I tried invoking the underlying D-Bus method directly I could see that the service was returning multiple account results for account #7 (output reindented for readability):

    $ gdbus call --session --dest com.ubuntu.OnlineAccounts.Manager \
                 --object-path /com/ubuntu/OnlineAccounts/Manager \
                 --method com.ubuntu.OnlineAccounts.Manager.GetAccounts {}
    ([(uint32 7, {'authMethod': <1>,
                  'displayName': <'<email address hidden>'>,
                  'serviceId': <'twitter-microblog'>,
                  'settings/friends/color': <'#05BBFC'>,
                  'settings/friends/send_enabled': <true>,
                  'settings/gwibber/color': <'#05BBFC'>,
                  'settings/gwibber/send_enabled': <true>}),
      (7, {'authMethod': <1>,
           'displayName': <'<email address hidden>'>,
           'serviceId': <'twitter-poll'>}),
      ...],)

So this clearly looks like a bug in the client library. It should be using (accountId, serviceId) as the key for managing it's local representation of the accounts.

[Alberto Mardegan (mardy)]

The new API has been built with the assumption that each client application will have access at most to one service per account.
See the last paragraph in
https://docs.google.com/document/d/1inf_HhRPdyERMBlET01baTnoeqeYXn0pUvzlBuE0m0k/edit#heading=h.ug99hyx6imji

Now, this means that the service should never return the same account ID twice, but you found a case where this is not true. This is probably a bug, but please read on.

The new API is more limited than the old one, and has been designed to support click apps and scopes. For these clients, we expect to be able to identify them by their app-id, either by inferring it from their apparmor label or (for unconfined processes) by receiving it as an explicit parameter.
Your gdbus invocation doesn't fall in either of these scenarios, because what we get is a request from an unconfined process which does not specify an "applicationId" parameter.

I believe that if you add such an "applicationId" parameter pointing to a valid application ID, you will not get any accounts listed twice. If that's not the case, please let me know.

Now, what is certainly true is that in this specific case the API does not behave in a deterministic way. I would suggest that we "morph" the bug about having an explicit D-Bus error when an unconfined client makes a request without specifying an "applicationId" parameter. WDYT?

[James Henstridge (jamesh)]

So for the code I'm working on, some components will be running confined and only have access to their own service IDs, while there will also be an unconfined process acting as a central registry that wants to know about the accounts that have active services for any of the related confined processes.

Are you seriously saying that I should use one API for some of these processes and a totally different one for the others? At the D-Bus level, everything seems to be doing what I want and would expect: it is just the client library getting in the way.

And from what you've described, my patch should have no impact to confined applications using the new API, since the D-Bus service will only return a single service per account there. This just makes the client library functional in the unconfined case rather than just acting unpredictably.

[Alberto Mardegan (mardy)]

You don't have to use a different API in the unconfined process: just do something like

  for (const QString &applicationId: supportedClients) {
      OnlineAccounts::Manager manager(applicationId);
      auto accounts = manager.availableAccounts();
      for (Account *account, accounts) {
          ... do stuff ...
      }
  }

The API can (and has been designed to) be used by unconfined clients, but always under the assumption that the unconfined client is acting on behalf of a proper click application, with its own ID and .application and .service files generated by the click hooks.

Now, I've given another quick review at the D-Bus API and at your MP, and I couldn't find any place that would break with your patch. The assumption that one click application/scope cannot use more than one service for the same account still holds, but that's enforced by the new click hooks, while the service and library can -- at first sight -- deal with multiple services, if your patch is applied.

Of course, given that I've kept that assumption in mind while writing the project, it's likely that there might be other places which break, and that your test case didn't uncover. It's for this reason that I'm hesitant to accept this.

So, my suggestion is to go with the pseudo code above. If for some reason that's not suitable (please explain), I'm ready to accept your patch, provided that it's landed in a single silo along with your project (so that we have a better confidence that this is the only needed changed, and that there aren't hidden dragons).

[James Henstridge (jamesh)]

So if I need to monitor N different services, I'll need to repeat the requests N different times, and set up N different signal connections to watch for changes? That seems a bit overboard when the D-Bus interface was designed so this could be handled in one go.

I agree that a change like this needs to be tested with existing users of the API. Do you have a list of such users so we can find out whether it actually causes a problem?

I'm kind of curious, since any application that receives multiple service IDs from the D-Bus daemon is almost certainly seeing unpredictable behaviour from the current version of the client library.

[Alberto Mardegan (mardy)]

This new API has only a couple of users, and I'm quite sure they'll work fine with your changes.

There's indeed an issue with the change notifications being sent to all clients (I just filed bug 1603412 about that), and possibly confusing them -- your patch should help in avoiding ending up in an inconsistent state.

About the pseudo code I suggested, indeed it's making a signal connection for each Manager object, but I suspect that given that they are all targeting the same "service/object path/interface" triplet they'll be squashed into a single connection in some layer below in the D-Bus stack, so they won't pose an additional burden on the D-Bus service.

Please note that, even if we add your patch (which is actually needed for solving the notification handling issue), the main -- and I would add, *only* -- goal of the library is to support clients represented by *single* .application file. Unconfined clients which perform the authentication on behalf of a *single* click application also fall into this category, but anything more complex than this doesn't.
So, given that your project doesn't fall in this category, you are welcome to use the library if it fits your goals, but if it doesn't, I'm not inclined to extend the API with functionality that would not be useful for a click application.

The reason I'm writing this is that I suspect that online-accounts-api (even with your patch applied) is still too limited and not suitable for your goal, unless you go for the pseudocode I wrote above: how will you find out the service IDs you are interested in?
Normally, they are encoded in the .application file shipped by the app, and online-accounts-api internally figures that out; but it doesn't expose any methods that would let you know what service types are supported by an application.

[Launchpad Janitor (janitor)]

This bug was fixed in the package online-accounts-api - 0.1+16.10.20160722.4-0ubuntu1

---------------
online-accounts-api (0.1+16.10.20160722.4-0ubuntu1) yakkety; urgency=medium

  [ Alberto Mardegan ]
  * Tests: keep the service alive while running the test (LP: #1603706)
  * Enable coverage reporting for the OnlineAccountsDaemon library

  [ James Henstridge ]
  * Add a constructor for OnlineAccounts::Manager that takes a custom D-
    Bus connection. (LP: #1602153)
  * Expose all serviceIds for each account rather than ignoring all but
    the first. (LP: #1602159)

 -- Michi Henning <email address hidden> Fri, 22 Jul 2016 05:09:23 +0000