Bug 34640

Summary: Use Protocol.IdentifyAccount to choose accounts' object paths
Product: Telepathy Reporter: Will Thompson <will>
Component: mission-controlAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Regression tests: use a simulated CM instead of just holding the bus name
Don't give storage plugins the full parameter set, just 'account'
Call IdentifyAccount to get new accounts' names
McdCreateAccountData: correctly free the user-data

Description Will Thompson 2011-02-23 14:55:04 UTC
The specification says that, when constructing a new account's object path (which is its persistent identifier):

  The account manager SHOULD choose acct such that if an account is deleted,
  its object path will be re-used if and only if the new account is in some
  sense "the same" (incorporating the 'account' parameter in some way is
  recommended)

MC doesn't really respect this all that well: it guesses based on the 'account' parameter, but this doesn't cut it for protocols like IRC.

Since spec version something recent, there's been a method that CMs can implement to turn a dictionary of parameters into a unique identifier for the account, suitable for MC to use when building the object path: <http://telepathy.freedesktop.org/spec/Protocol.html#Method:IdentifyAccount>. MC should use it, where possible. This also allows MC to prevent you creating two copies of the same account.

For backwards compatibility, the object paths for existing accounts should be preserved, but we could stash the CM's preferred identifier alongside the account to avoid having to repeatedly retrieve it as discussed on bug 34416.
Comment 1 Simon McVittie 2013-10-15 14:25:42 UTC
Hijacking this a bit: ExternalPasswordStorage and AccountStorage, which are related to this, are *still* drafts...

(In reply to comment #8)
> Actually, no, they're still marked as DRAFT in the spec. Let's just undraft
> them: the implementation in MC isn't great, but it'll do.

I noticed they don't have any regression test coverage, started writing a test, ran into "ugh, how much of a CM do I need to implement?" and lost interest for now.

I'd like to fix the fact that it calls IdentifyAccount() repeatedly - I think it ought to call it exactly once, on account creation, and save the result in the account, as Will said on Bug #34416.

I wonder whether telepathy-haze should even persist libpurple accounts on disk, and use the AccountStorage interface to manage them... although it would probably have to make sure that all accounts are disabled at the libpurple level before it connects any of them.
Comment 2 Simon McVittie 2013-10-15 17:14:25 UTC
Created attachment 87679 [details] [review]
Regression tests: use a simulated CM instead of just  holding the bus name

We'll need this if we want to call IdentifyAccount.

---

Actually, with a more gradual approach (not trying to solve AccountStorage immediately), it isn't so bad.
Comment 3 Simon McVittie 2013-10-15 17:14:48 UTC
Created attachment 87680 [details] [review]
Don't give storage plugins the full parameter set, just  'account'

We broke plugin API since the last release anyway, so this isn't
a new API break.

The doc-comments all claim that the string is the result of
IdentifyAccount, because that's about to be true. :-)
Comment 4 Simon McVittie 2013-10-15 17:15:28 UTC
Created attachment 87681 [details] [review]
Call IdentifyAccount to get new accounts' names

It's exposed through the plugin API so that exemplary plugins can
use the same utility functions to decide how to name accounts for
the ::created signal, if they want to.
Comment 5 Simon McVittie 2013-10-15 17:15:45 UTC
Created attachment 87682 [details] [review]
McdCreateAccountData: correctly free the user-data

In theory this is a bugfix, but nothing within MC actually calls
this function with a non-NULL GDestroyNotify anyway, and it isn't API.
Comment 6 Simon McVittie 2013-10-15 17:17:46 UTC
Comment on attachment 87680 [details] [review]
Don't give storage plugins the full parameter set, just  'account'

Review of attachment 87680 [details] [review]:
-----------------------------------------------------------------

::: mission-control-plugins/account.c
@@ +287,4 @@
>   * MC has not previously seen before (ie one created by a 3rd party
>   * in the back-end that the plugin in question provides an interface to).
>   *
> + * Changed in 5.17: instead of a map from string to GValue, the last

Yeah in principle this should have been 5.UNRELEASED, but if we can't get this merged before we next stable-branch, then we're not trying hard enough.

::: src/mcd-account-manager-sso.c
@@ +971,5 @@
>        g_hash_table_insert (params, g_strdup (MC_ACCOUNT_KEY), &value);
>  
> +      /* FIXME: We should call IdentifyAccount(params) really. But this
> +       * plugin probably doesn't even compile any more, so, whatever;
> +       * just use the "account name". */

If people want to just delete this (pseudo-)plugin, I'd be in favour.
Comment 7 Simon McVittie 2013-10-15 17:26:40 UTC
Comment on attachment 87681 [details] [review]
Call IdentifyAccount to get new accounts' names

Review of attachment 87681 [details] [review]:
-----------------------------------------------------------------

::: configure.ac
@@ +220,4 @@
>  AC_DEFINE([TP_DISABLE_SINGLE_INCLUDE], [], [Avoid individual headers])
>  
>  PKG_CHECK_MODULES([GLIB],
> +	[glib-2.0 >= 2.36, gobject-2.0, gmodule-no-export-2.0, gio-2.0])

GTask because life's too short.

::: mission-control-plugins/account.c
@@ +312,5 @@
> +void
> +mcp_account_manager_identify_account_async (McpAccountManager *mcpa,
> +    const gchar *manager,
> +    const gchar *protocol,
> +    GVariant *parameters,

GVariant because I don't want to add any new API that's shaped like dbus-glib. I should probably add some doc-comments to these.

::: src/mcd-storage.c
@@ +622,5 @@
> +    {
> +      g_task_return_pointer (task, g_strdup (identification), g_free);
> +    }
> +  else if (g_error_matches (error, TP_ERROR, TP_ERROR_NOT_IMPLEMENTED) ||
> +      g_error_matches (error, DBUS_GERROR, DBUS_GERROR_SERVICE_UNKNOWN))

I want to trap ServiceUnknown and either raise a different error or carry on with the "can't identify" code path, because it seems bad if MC raises ServiceUnknown in response to a CreateAccount call - that's indistinguishable from MC itself not existing!

tests/twisted/account-manager/bad-cm.py wants this to return NotImplemented, which is raised by _mcd_account_set_parameters. The spec says so.

If the CM raises InvalidArgument from IdentifyAccount (which is undocumented, but reasonable), we'll pass that on as the result of CreateAccount, which is what the spec says we should do.

If the CM raises InvalidHandle (which is undocumented, but is a reasonable way to represent "that identifier is bad"), we'll pass *that* on as the result of CreateAccount, which is undocumented but reasonable.

I'm tempted to document InvalidArgument and InvalidHandle as possible errors raised by IdentifyAccount, and InvalidHandle as a possible error raised by CreateAccount (with InvalidHandle having its usual secondary meaning of "invalid identifier-that-is-equivalent-to-a-handle").
Comment 8 Guillaume Desmottes 2013-10-15 21:29:58 UTC
Comment on attachment 87679 [details] [review]
Regression tests: use a simulated CM instead of just  holding the bus name

Review of attachment 87679 [details] [review]:
-----------------------------------------------------------------

++
Comment 9 Guillaume Desmottes 2013-10-15 21:31:21 UTC
Comment on attachment 87680 [details] [review]
Don't give storage plugins the full parameter set, just  'account'

Review of attachment 87680 [details] [review]:
-----------------------------------------------------------------

++

::: src/mcd-account-manager-sso.c
@@ +971,5 @@
>        g_hash_table_insert (params, g_strdup (MC_ACCOUNT_KEY), &value);
>  
> +      /* FIXME: We should call IdentifyAccount(params) really. But this
> +       * plugin probably doesn't even compile any more, so, whatever;
> +       * just use the "account name". */

Fine with me, we have a more recent plugin in Empathy anyway.
Comment 10 Guillaume Desmottes 2013-10-15 21:38:56 UTC
Comment on attachment 87681 [details] [review]
Call IdentifyAccount to get new accounts' names

Review of attachment 87681 [details] [review]:
-----------------------------------------------------------------

Fine with me. ++
Comment 11 Guillaume Desmottes 2013-10-15 21:40:35 UTC
Comment on attachment 87682 [details] [review]
McdCreateAccountData: correctly free the user-data

Review of attachment 87682 [details] [review]:
-----------------------------------------------------------------

++
Comment 12 Simon McVittie 2013-10-29 11:30:22 UTC
Fixed in git for 5.17.0, thanks

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.