service() method returns invalid service instance

[Michi Henning]

I have a Google Drive account configured. When I call GetAccounts(), the account is returned, and calling the service() method on the account returns a Service instance. But that instance returns false from isValid(), its id(), displayName(), and iconSource() methods return an empty string.

[Alberto Mardegan (mardy)]

Please attach the dbus-monitor (or equivalent app) logs.

[Michi Henning (michihenning)]

Here is the relevant code:

    QList<storage::internal::AccountDetails> accounts;
    for (auto const& acct : manager_.availableAccounts())
    {
        auto const it = BUS_NAMES.find(acct->serviceId());
        if (it == BUS_NAMES.end())
        {
            continue;
        }

        qDebug() << "using" << acct->serviceId();
        auto service = acct->service();
        qDebug() << "valid:" << service.isValid();
        qDebug() << "id:" << service.id();
        qDebug() << "icon:" << service.iconSource();
        qDebug() << "displayName:" << service.displayName();
        storage::internal::AccountDetails ad;
        ad.busName = it->second.bus_name;
        ad.objectPath = QDBusObjectPath(QStringLiteral("/provider/%1").arg(acct->id()));
        ad.id = acct->id();
        ad.serviceId = acct->serviceId();
        ad.providerName = it->second.provider_name;
        ad.iconName = "";

        accounts.append(ad);
    }

When I run this, it prints:

storage-framework-registry: [16:58:50.030] using "google-drive-scope"
storage-framework-registry: [16:58:50.031] valid: false
storage-framework-registry: [16:58:50.031] id: ""
storage-framework-registry: [16:58:50.031] icon: QUrl("")
storage-framework-registry: [16:58:50.032] displayName: ""

[Michi Henning (michihenning)]

And this is the relevant struct as reported by dbus-monitor:

      struct {
         uint32 1
         array [
            dict entry(
               string "authMethod"
               variant int32 2
            )
            dict entry(
               string "displayName"
               variant string "<email address hidden>"
            )
            dict entry(
               string "serviceId"
               variant string "google-drive-scope"
            )
            dict entry(
               string "settings/auth/oauth2/web_server/ClientId"
               variant string "401977606506-2f9ubd66m8822uhrhiq3ka7k81i76bsk.apps.googleusercontent.com"
            )
            dict entry(
               string "settings/auth/oauth2/web_server/ClientSecret"
               variant string "Vx_vflfl2tT6NV18BDETcNRC"
            )
            dict entry(
               string "settings/auth/oauth2/web_server/RedirectUri"
               variant string "https://wiki.ubuntu.com"
            )
            dict entry(
               string "settings/auth/oauth2/web_server/Scope"
               variant array [
                     string "https://www.googleapis.com/auth/drive.readonly"
                  ]
            )
         ]
      }

[Alberto Mardegan (mardy)]

Thanks Michi, but I really need to see the parameters for the GetAccount call, to see what applicationId you are passing.
Besides that, I see that the D-Bus reply you pasted does not include the array of services: there isn't even an *empty* array, which suggests that the libonline-accounts-daemon1 you have installed is not the latest one. Can you please check, what version you have installed?

[Michi Henning (michihenning)]

Except for the trace I added, the code is in this branch:

https://code.launchpad.net/~michihenning/storage-framework/registry

The source file is here:

http://bazaar.launchpad.net/~michihenning/storage-framework/registry/view/head:/src/registry/internal/ListAccountsHandler.cpp

I have version 0.1+17.04.20161110-0ubuntu1 of online-accounts-daemon1.

[Alberto Mardegan (mardy)]

As I wrote, you need to specify an application ID when instantiating the manager:

https://bugs.launchpad.net/ubuntu/+source/online-accounts-api/+bug/1638769/comments/7

[Michi Henning (michihenning)]

We don't have an application ID because the request comes from the registry, which acts on behalf of all sorts of applications.

I don't understand the need for an ID. Right now, the getAccounts() method returns me the full list of accounts. I know that I have a google-drive account, an mcloud account, etc. For each of these, I can see the id, the display name, and the service ID. Yet, the API refuses to let me see the string "Google Drive" and the icon name. Why?

All we need are three things:

- the translated provider name
- the icon name
- the dbus name

Why hide those from us?

[Alberto Mardegan (mardy)]

If you are acting on behalf of other applications, you should use their applicationId; but in your case I believe that you really want your own applicationId. Let me try to explain why it's needed.

In the Online Accounts panel in the system settings, when the user clicks on one existing account, he'll see what applications can use this account, and he's got a chance to enable or disable them.

Now, you need to decide what the user should see in there, and how you want to present the storage-framework to the user: I think that the user should see something like "[icon] File synchronisation [enable/disable]", right?
This "File synchronisation" string, along with the icon next to it, comes from an .application file (it can be optionally overridden by a .service file, for those cases where you want to have different icons/text for different accounts); either storage-framework or some other app needs to provide it, and storage-framework needs to use this application-id to find out whether the synchronisation is enabled for that account.

[Michi Henning (michihenning)]

Hi Alberto, thanks for the quick reply!

The user shouldn't see storage framework when they authorize things in Online Accounts. The user doesn't even know what a storage service is. Besides, storage framework will be used for lots more things than just synchronisation. Eventually, for each individual application, the user will be able to decide whether or not to use some cloud provider for that application.

For example, I might decide to have the camera app store photos on Dropbox, but have the music app use music in both Dropbox and Google Drive.

What we *do* need to do is present the list of available storage providers to the user, so we can create a selection dialog for the user to decide which accounts(s) to store photos in, music in, etc.

We really need this list, and we really need to get at the dbus name, icon name, and translated provider name, otherwise we can't present the list to the user, and we don't know what dbus name to connect to.

Again, I don't see why the Accounts API provides me with the list of all available accounts, but won't let me know these three things about each account (while letting me know other things).

[Alberto Mardegan (mardy)]

Hi Michi, I guess you set the bug status to invalid by mistake?

OK, it looks like I had a very wrong idea of how the storage framework would work. So let me recap my understanding, and please tell me if I got it wrong.

You have your own UI to let the user configure which accounts to use, and to configure what data should be stored there. Applications will not have access to the account information, they will not know to which account the data is being synchronised to and (most important) they won't get any oauth token or other type of credentials, because all the talking with the remote service is done by the storage-framework.

Still, someone needs to add the accounts, and request access to them. When Online Accounts receives a request for a new account to be created, or to grant access to an existing account to a new app, we show the name and icon of that application (see https://wiki.ubuntu.com/OnlineAccounts#App_access ).

We need to sort out what should be shown there. If the application (for instance, the camera app) is not getting direct access to the account, then it seems to me that presenting this request as coming from a "File synchronisation service" is the proper thing to do.

Can you point me at some design documents from the storage framework? Maybe if I saw them I'd understand better how to help you.

[Michi Henning (michihenning)]

Sorry about the status change, that must have been a mouse click that went astray :-(

You are correct in that applications won't get any oauth tokens or the like. That's all handled by the storage framework. We pass the oauth token into the *provider* implementation, so the provider can authenticate with the cloud service; clients never come near the token.

You are right, the accounts are not going to get there by magic. Users will still have to add a cloud account to online accounts as before. But we need to allow the user to select which cloud service (and account within that service) will be used by a particular application (such as the camera app) to access/store content.

There are some design documents, but not anything that would be useful here, it seems. Basically, we have a registry DBus service. The point of that service is to deliver information about which accounts the user has created within Online accounts. Our client-side runtime accesses the registry (invisibly to the calling application code) to find out which accounts exist and are potentially available as a storage service in the cloud. (We will post-filter all the account info we get from online accounts to show only accounts that actually represent a storage provider.) We will show a selection dialog at some point via a trusted helper or some such, where the user can select which cloud providers(s) to use for application X. What we need to present in that list is:

- The translated name of the cloud service, such as "Google Drive" or "China Mobile mCloud",

- the name of the icon for the service, so we can put up a pretty picture on the screen,

- the dbus name of the provider that belongs to the account, so we know which provider we need to talk to for that account.

- the displayName (which we get from online accounts already)

That tells the user: "the candidate storage provider you can select from are:

- "Google Drive", <pretty icon>, "<email address hidden>"
- "Google Drive", <pretty icon>, "<email address hidden>"
- "Chine Mobile mCloud", <pretty icon>, "<email address hidden>"

etc.

That's pretty much all we need for the moment.

Online accounts as it stands gives us all but the translated name, the icon name, and the dbus name when we pass an empty application ID.

[Alberto Mardegan (mardy)]

Thanks for taking the time to explain the idea. :-)

I have to tell you, that the new online-accounts-api is simpler than the
other API we have, and it's so in virtue of its more limited scope: it
was initially designed for click applications, or for unconfined
services acting on behalf of a specific click application.

I'm quite confident that we can make it work for your use-case, but some
compromises might be needed.

The more you explain the goal, the more I get convinced that the storage
framework needs to be registered as an app with Online Accounts. Once we
can properly identify the storage-framework as the initiator, we can
then make some tweaks on the UI on our side, if there is the need to;
for example, if we need to customize the access request screen we show
to the user when creating/authorising a new account (which, currently
shows icon and name of the requesting app, which might not be what we
want in this case).

In order to do the above, you need to ship the .application file I've
mentioned before. The fact that the API works without specifying a valid
applicationId is not intentional, and is in fact a bug.

Please see
https://bugs.launchpad.net/ubuntu/+source/online-accounts-api/+bug/1638769/comments/7
and let me know if you think there's something wrong in all this. :-)

[James Henstridge (jamesh)]

For what it is worth, I think there is value in keeping the blank/no app ID mode working for the benefit of trusted helpers that need to work on behalf of a number of confined processes using different AppArmor labels. You could special case storage-framework, but I doubt we'll be the last to want this sort of access (heck, we're one of the first pieces of code using this new API).

Given that everything else in the API works with a blank app ID, I would tend to see the fact that this doesn't work as a bug rather than a result of undefined behaviour.

And looking at the D-Bus API, this still seems like a weird way to package up the information we need. At the moment, there are 3 ways to receive information about accounts:

 1. all accounts from the GetAccounts method.
 2. a changed account from the AccountChanged signal
 3. one account from RequestAccess method.

As it is now, the service information is only returned by GetAccounts, with no information being returned if an app ID hasn't been supplied. Given that AccountInfo already included an a{sv} dictionary, stashing the information there seems like a simpler solution. It would have ensured the information was available whichever way the client learned about the account, and it would piggyback on the filtering already performed on the account list.

[Alberto Mardegan (mardy)]

On 30/11/2016 14:49, James Henstridge wrote:
> For what it is worth, I think there is value in keeping the blank/no app
> ID mode working for the benefit of trusted helpers that need to work on
> behalf of a number of confined processes using different AppArmor
> labels. You could special case storage-framework, but I doubt we'll be
> the last to want this sort of access (heck, we're one of the first
> pieces of code using this new API).

Your use-case is a bit on the edge, but AFAIU it should be supported. A
service that authenticates on behalf of other apps should instantiate an
AccountManager for each app, specifying the app's ID, and this should
just work.
However, your case is a bit different, in that -- I believe -- you don't
want the client apps to have any knowledge of online accounts (you
handle it transparently for them) and you handle the app permissions
yourself. The fact that the user has to go through the Online Accounts
trusted prompt in order to authorise the storage-framework to use a
certain account is probably more an annoyance than a desired experience.
In practice, you are not simply acting as a bridge between the client
apps and OA, but you are actively shielding these parts from knowing
about each other: the apps won't see anything from OA, and OA won't see
anything from the app. That's fine, but since OA needs to know about its
client, it follows that storage-framework needs to identify itself. Once
you do that, then we can even special-case it and change the UI flow
just for you, if that's needed.

> Given that everything else in the API works with a blank app ID, I would
> tend to see the fact that this doesn't work as a bug rather than a
> result of undefined behaviour.

Not really: between adding complexity to the code of the OA service and
you adding an XML file to your project (which you'll need to add anyway,
to get our trust prompt working properly), I'm not scratching my head to
decide which one is better. :-)

> And looking at the D-Bus API, this still seems like a weird way to
> package up the information we need. At the moment, there are 3 ways to
> receive information about accounts:
>
> 1. all accounts from the GetAccounts method.
> 2. a changed account from the AccountChanged signal
> 3. one account from RequestAccess method.
>
> As it is now, the service information is only returned by GetAccounts,
> with no information being returned if an app ID hasn't been supplied.
> Given that AccountInfo already included an a{sv} dictionary, stashing
> the information there seems like a simpler solution. It would have
> ensured the information was available whichever way the client learned
> about the account, and it would piggyback on the filtering already
> performed on the account list.

That's another issue, please let's discuss it separately. But just briefly:
* You might have just providers, with no accounts created
* The client lib starts by calling GetAccounts (and any alternative
implementation should do the same), so the info is already all there; no
need to resend it again later.

It's not settled on stone, we might have to add a ProvidersChanged
signal for when new providers are installed, but ...