[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 #3 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2012-07-18 14:16:35 PDT ---
(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.

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

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.

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.

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