[Bug 52231] Storage plugins should be able to create new accounts

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=52231

--- Comment #4 from Xavier Claessens <xclaesse at gmail.com> 2012-07-18 15:40:39 PDT ---
(In reply to comment #3)
> (In reply to comment #2)
> > How MC currently store new account is by creating a unique_name, then telling
> > the storage all the key/value to save for that unique_name. Of course as none
> > of the plugins will know that unique_name they all will return FALSE except for
> > the default storage.
> 
> Because the storage API is rather weird, this isn't actually enforced or
> anything.
> 
> I think I'd prefer to say that create_account is expected to store the
> connection manager name and protocol (but nothing else), and replace the calls
> to mcp_account_manager_get_unique_name,
> mcd_storage_set_string(MC_ACCOUNTS_KEY_MANAGER) and
> mcd_storage_set_string(MC_ACCOUNTS_KEY_PROTOCOL) in
> _mcd_account_manager_create_account with this pseudocode:
> 
> if (provider != NULL)
> {
>     McpAccountStorage *storage = find_storage_for_provider (provider);
> 
>     unique_name = mcp_account_storage_create_account (storage, manager,
>         protocol, ..., error);
>     if (unique_name == NULL)
>     {
>         raise error;
>         return;
>     }
> }
> else
> {
>     foreach McpAccountStorage storage, highest priority first
>     {
>         unique_name = mcp_account_storage_create_account (storage, manager,
>             protocol, ..., error);
>         if (unique_name != NULL)
>         {
>             if (mcd_storage_get_plugin (priv->plugin_manager) != storage)
>             {
>                 mcp_account_storage_delete_all (storage, unique_name);
>                 raise NotAvailable, "plugin tried to use a name that already
> exists";
>             }
> 
>             /* success */
>             break;
>         }
>         free error;
> 
>         if (unique_name == NULL)
>             raise NotAvailable, "No backend was willing to create that
> account";
>     }
> }
> 
> where the default implementation of mcp_account_storage_create_account
> (inherited by all out-of-tree plugins and by the default plugin) is this
> (pseudocode):
> 
> gboolean create_account (..., error)
> {
>     unique_name = mcp_account_manager_get_unique_name (...);
> 
>     if (unique_name == NULL)
>         raise NotAvailable, "Could not construct account name";
> 
>     if (!mcp_account_storage_set (self, am, unique_name,
>             MC_ACCOUNTS_KEY_MANAGER, manager) ||
>         !mcp_account_storage_set (self, am, unique_name,
>             MC_ACCOUNTS_KEY_PROTOCOL, protocol)
>         raise NotAvailable, "Storage backend could not store new account";
> 
>     return TRUE;
> }
> 
> Then we preserve current functionality for all current plugins, including
> out-of-tree.

Good idea!

> > It also let the plugin decide of the unique_name to use for that account, so
> > for example storage "Badger" could want to prefix all account names with
> > "badger_"
> 
> According to the Tp spec, the account's unique name must be of the form
> manager/protocol/x for some x. Why would a backend ever want to change the
> value of x, given that it's opaque?
> 
> (One possible reason would be "to set x to badger_123 where 123 is its internal
> identifier for that account", I suppose.)

That's exactly what GOA plugin does. Also having manager/protocol/goa_foo makes
debugging easier as you can tell from the object path which are the accounts
your plugin is managing :)

> Letting the plugin decide the unique name raises the possibility of plugins
> that generate a unique name that collides with one used by a lower- or
> higher-priority plugin, if they don't use mcp_account_manager_get_unique_name
> (which checks for collisions automatically). In the pseudocode above, that's
> partly handled, but unfortunately it needs a new mcp_account_storage_delete_all
> method, and it still doesn't handle the case of a plugin stealing a name from a
> lower-priority one.

_delete_all() already exists as _delete() the NULL key, no?

> Another way to do this would be to provide a
> mcp_account_manager_account_exists(McpAccountManager, unique_name) method,
> document that create_account implementations that do not use
> mcp_account_manager_get_unique_name are expected to call
> mcp_account_manager_account_exists and not use names for which that returns
> TRUE, and make failure to do so into a programming error (i.e. g_critical). A
> badly-written plugin can crash us anyway, so that's not a regression.

I think it is plugin's responsability to ensure uniqueness, either by using
mcp_account_manager_get_unique_name() or by having its own prefix (e.g. "goa_")
+ an internal identifier. If the unique_name given by a plugin already exists,
MC will already warn and discard that account, IIRC that's somewhere in
McdAccount.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
You are the assignee for the bug.