McdAccount: migrate avatars to a XDG path

This is the easy part of XDGification: there is no plugin involvement or
anything for avatars (yet!), it's just hard-coded.

Changing the avatar location from a single directory to a search path
results in some new corner cases, so test them.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=35896
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

5 files changed, 364 insertions, 49 deletions

diff --git a/src/mcd-account.c b/src/mcd-account.c
index ecc10358..256575b1 100644
--- a/src/mcd-account.c
+++ b/src/mcd-account.c
@@ -24,6 +24,7 @@
 #include "config.h"
 #include "mcd-account.h"
 
+#include <errno.h>
 #include <stdio.h>
 #include <string.h>
 
@@ -51,7 +52,7 @@
 #include "_gen/cli-Connection_Manager_Interface_Account_Storage-body.h"
 
 #define MAX_KEY_LENGTH (DBUS_MAXIMUM_NAME_LENGTH + 6)
-#define MC_AVATAR_FILENAME	"avatar.bin"
+#define MC_OLD_AVATAR_FILENAME	"avatar.bin"
 
 #define MCD_ACCOUNT_PRIV(account) (MCD_ACCOUNT (account)->priv)
 
@@ -673,7 +674,7 @@ load_manager (McdAccount *account)
 /* Returns the data dir for the given account name.
  * Returned string must be freed by caller. */
 static gchar *
-get_account_data_path (McdAccountPrivate *priv)
+get_old_account_data_path (McdAccountPrivate *priv)
 {
     const gchar *base;
 
@@ -759,7 +760,7 @@ mcd_account_delete (McdAccount *account,
 
     mcd_storage_delete_account (priv->storage, name);
 
-    data_dir_str = get_account_data_path (priv);
+    data_dir_str = get_old_account_data_path (priv);
 
     if (data_dir_str != NULL)
     {
@@ -2811,6 +2812,213 @@ register_dbus_service (McdAccount *self,
 					     (GObject *) self);
 }
 
+/*
+ * @account: (allow-none):
+ * @dir_out: (out): e.g. ~/.local/share/telepathy/mission-control
+ * @basename_out: (out): e.g. gabble-jabber-fred_40example_2ecom.avatar
+ * @file_out: (out): @dir_out + "/" + @basename_out
+ */
+static void
+get_avatar_paths (McdAccount *account,
+                  gchar **dir_out,
+                  gchar **basename_out,
+                  gchar **file_out)
+{
+    gchar *dir = NULL;
+
+    dir = g_build_filename (g_get_user_data_dir (),
+                            "telepathy", "mission-control", NULL);
+
+    if (account == NULL)
+    {
+        if (file_out != NULL)
+            *file_out = NULL;
+
+        if (basename_out != NULL)
+            *basename_out = NULL;
+    }
+    else if (basename_out != NULL || file_out != NULL)
+    {
+        gchar *basename = NULL;
+
+        basename = g_strdup_printf ("%s.avatar", account->priv->unique_name);
+        g_strdelimit (basename, "/", '-');
+
+        if (file_out != NULL)
+            *file_out = g_build_filename (dir, basename, NULL);
+
+        if (basename_out != NULL)
+            *basename_out = basename;
+        else
+            g_free (basename);
+    }
+
+    if (dir_out != NULL)
+        *dir_out = dir;
+    else
+        g_free (dir);
+}
+
+static gboolean
+save_avatar (McdAccount *self,
+             gpointer data,
+             gssize len,
+             GError **error)
+{
+    gchar *dir = NULL;
+    gchar *file = NULL;
+    gboolean ret = FALSE;
+
+    get_avatar_paths (self, &dir, NULL, &file);
+
+    if (mcd_ensure_directory (dir, error) &&
+        g_file_set_contents (file, data, len, error))
+    {
+        DEBUG ("Saved avatar to %s", file);
+        ret = TRUE;
+    }
+    else if (len == 0)
+    {
+        GArray *avatar = NULL;
+
+        /* It failed, but maybe that's OK, since we didn't really want
+         * an avatar anyway. */
+        _mcd_account_get_avatar (self, &avatar, NULL);
+
+        if (avatar == NULL)
+        {
+            /* Creating the empty file failed, but it's fine, since what's
+             * on disk correctly indicates that we have no avatar. */
+            DEBUG ("Ignoring failure to write empty avatar");
+
+            if (error != NULL)
+                g_clear_error (error);
+        }
+        else
+        {
+            /* Continue to raise the error: we failed to write a 0-byte
+             * file into the highest-priority avatar directory, and we do
+             * need it, since there is a non-empty avatar in either that
+             * directory or a lower-priority directory */
+            g_array_free (avatar, TRUE);
+        }
+    }
+
+    g_free (dir);
+    g_free (file);
+    return ret;
+}
+
+static gchar *_mcd_account_get_old_avatar_filename (McdAccount *account,
+                                                    gchar **old_dir);
+
+static void
+mcd_account_migrate_avatar (McdAccount *account)
+{
+    GError *error = NULL;
+    gchar *old_file;
+    gchar *old_dir = NULL;
+    gchar *new_dir = NULL;
+    gchar *basename = NULL;
+    gchar *new_file = NULL;
+    gchar *contents = NULL;
+    guint i;
+
+    /* Try to migrate the avatar to a better location */
+    old_file = _mcd_account_get_old_avatar_filename (account, &old_dir);
+
+    if (!g_file_test (old_file, G_FILE_TEST_EXISTS))
+    {
+        /* nothing to do */
+        goto finally;
+    }
+
+    DEBUG ("Migrating avatar from %s", old_file);
+
+    get_avatar_paths (account, &new_dir, &basename, &new_file);
+
+    if (g_file_test (new_file, G_FILE_TEST_IS_REGULAR))
+    {
+        DEBUG ("... already migrated to %s", new_file);
+
+        if (g_unlink (old_file) != 0)
+        {
+            DEBUG ("Failed to unlink %s: %s", old_file,
+                   g_strerror (errno));
+        }
+
+        goto finally;
+    }
+
+    if (!mcd_ensure_directory (new_dir, &error))
+    {
+        DEBUG ("%s", error->message);
+        goto finally;
+    }
+
+    if (g_rename (old_file, new_file) == 0)
+    {
+        DEBUG ("Renamed %s to %s", old_file, new_file);
+    }
+    else
+    {
+        gsize len;
+
+        DEBUG ("Unable to rename %s to %s, will try copy+delete: %s",
+               old_file, new_file, g_strerror (errno));
+
+        if (!g_file_get_contents (old_file, &contents, &len, &error))
+        {
+            DEBUG ("Unable to load old avatar %s: %s", old_file,
+                   error->message);
+            goto finally;
+        }
+
+        if (g_file_set_contents (new_file, contents, len, &error))
+        {
+            DEBUG ("Copied old avatar from %s to %s", old_file, new_file);
+        }
+        else
+        {
+            DEBUG ("Unable to save new avatar %s: %s", new_file,
+                   error->message);
+            goto finally;
+        }
+
+        if (g_unlink (old_file) != 0)
+        {
+            DEBUG ("Failed to unlink %s: %s", old_file, g_strerror (errno));
+            goto finally;
+        }
+    }
+
+    /* old_dir is typically ~/.mission-control/accounts/gabble/jabber/badger0.
+     * We want to delete badger0, jabber, gabble, accounts if they are empty.
+     * If they are not, we'll just get ENOTEMPTY and stop. */
+    for (i = 0; i < 4; i++)
+    {
+        gchar *tmp;
+
+        if (g_rmdir (old_dir) != 0)
+        {
+            DEBUG ("Failed to rmdir %s: %s", old_dir, g_strerror (errno));
+            goto finally;
+        }
+
+        tmp = g_path_get_dirname (old_dir);
+        g_free (old_dir);
+        old_dir = tmp;
+    }
+
+finally:
+    g_clear_error (&error);
+    g_free (basename);
+    g_free (new_file);
+    g_free (new_dir);
+    g_free (contents);
+    g_free (old_file);
+}
+
 static gboolean
 mcd_account_setup (McdAccount *account)
 {
@@ -3100,6 +3308,7 @@ _mcd_account_constructed (GObject *object)
 
     DEBUG ("%p (%s)", object, account->priv->unique_name);
 
+    mcd_account_migrate_avatar (account);
     mcd_account_setup (account);
 }
 
@@ -3633,32 +3842,27 @@ _mcd_account_set_avatar (McdAccount *account, const GArray *avatar,
 {
     McdAccountPrivate *priv = MCD_ACCOUNT_PRIV (account);
     const gchar *account_name = mcd_account_get_unique_name (account);
-    gchar *data_dir, *filename;
 
     DEBUG ("called");
-    data_dir = get_account_data_path (priv);
-    filename = g_build_filename (data_dir, MC_AVATAR_FILENAME, NULL);
-    if (!g_file_test (data_dir, G_FILE_TEST_EXISTS))
-	g_mkdir_with_parents (data_dir, 0700);
-    _mcd_chmod_private (data_dir);
-    g_free (data_dir);
 
     if (G_LIKELY(avatar) && avatar->len > 0)
     {
-	if (!g_file_set_contents (filename, avatar->data,
-				  (gssize)avatar->len, error))
+        if (!save_avatar (account, avatar->data, avatar->len, error))
 	{
-	    g_warning ("%s: writing to file %s failed", G_STRLOC,
-		       filename);
-	    g_free (filename);
+	    g_warning ("%s: writing avatar failed", G_STRLOC);
 	    return FALSE;
 	}
     }
     else
     {
-	g_remove (filename);
+        /* We implement "deleting" an avatar by writing out a zero-length
+         * file, so that it will override lower-priority directories. */
+        if (!save_avatar (account, "", 0, error))
+        {
+            g_warning ("%s: writing empty avatar failed", G_STRLOC);
+            return FALSE;
+        }
     }
-    g_free (filename);
 
     if (mime_type != NULL)
         mcd_storage_set_string (priv->storage,
@@ -3701,7 +3905,37 @@ _mcd_account_set_avatar (McdAccount *account, const GArray *avatar,
     return TRUE;
 }
 
-static gchar *_mcd_account_get_avatar_filename (McdAccount *account);
+static GArray *
+load_avatar_or_warn (const gchar *filename)
+{
+    GError *error = NULL;
+    gchar *data = NULL;
+    gsize length;
+
+    if (g_file_get_contents (filename, &data, &length, &error))
+    {
+        if (length > 0 && length < G_MAXUINT)
+        {
+            GArray *ret;
+
+            ret = g_array_new (FALSE, FALSE, 1);
+            g_array_append_vals (ret, data, (guint) length);
+            return ret;
+        }
+        else
+        {
+            DEBUG ("avatar %s was empty or ridiculously large (%"
+                   G_GSIZE_FORMAT " bytes)", filename, length);
+            return NULL;
+        }
+    }
+    else
+    {
+        DEBUG ("error reading %s: %s", filename, error->message);
+        g_error_free (error);
+        return NULL;
+    }
+}
 
 void
 _mcd_account_get_avatar (McdAccount *account, GArray **avatar,
@@ -3709,6 +3943,7 @@ _mcd_account_get_avatar (McdAccount *account, GArray **avatar,
 {
     McdAccountPrivate *priv = MCD_ACCOUNT_PRIV (account);
     const gchar *account_name = mcd_account_get_unique_name (account);
+    gchar *basename;
     gchar *filename;
 
     if (mime_type != NULL)
@@ -3720,29 +3955,37 @@ _mcd_account_get_avatar (McdAccount *account, GArray **avatar,
 
     *avatar = NULL;
 
-    filename = _mcd_account_get_avatar_filename (account);
+    get_avatar_paths (account, NULL, &basename, &filename);
 
-    if (filename && g_file_test (filename, G_FILE_TEST_EXISTS))
+    if (g_file_test (filename, G_FILE_TEST_EXISTS))
     {
-	GError *error = NULL;
-	gchar *data = NULL;
-	gsize length;
-	if (g_file_get_contents (filename, &data, &length, &error))
-	{
-	    if (length > 0 && length < G_MAXUINT) 
-	    {
-		*avatar = g_array_new (FALSE, FALSE, 1);
-		(*avatar)->data = data;
-		(*avatar)->len = (guint)length;
-	    }
-	}
-	else
-	{
-            DEBUG ("error reading %s: %s", filename, error->message);
-	    g_error_free (error);
-	}
+        *avatar = load_avatar_or_warn (filename);
     }
+    else
+    {
+        const gchar * const *iter;
+
+        for (iter = g_get_system_data_dirs ();
+             iter != NULL && *iter != NULL;
+             iter++)
+        {
+            gchar *candidate = g_build_filename (*iter, "telepathy",
+                                                 "mission-control",
+                                                 basename, NULL);
+
+            if (g_file_test (candidate, G_FILE_TEST_EXISTS))
+            {
+                *avatar = load_avatar_or_warn (candidate);
+                g_free (candidate);
+                break;
+            }
+
+            g_free (candidate);
+        }
+    }
+
     g_free (filename);
+    g_free (basename);
 }
 
 GPtrArray *
@@ -4203,15 +4446,14 @@ _mcd_account_get_keyfile (McdAccount *account)
 }
 
 static gchar *
-_mcd_account_get_avatar_filename (McdAccount *account)
+_mcd_account_get_old_avatar_filename (McdAccount *account,
+                                      gchar **old_dir)
 {
     McdAccountPrivate *priv = account->priv;
-    gchar *data_dir, *filename;
+    gchar *filename;
 
-    data_dir = get_account_data_path (priv);
-    DEBUG("data dir: %s", data_dir);
-    filename = g_build_filename (data_dir, MC_AVATAR_FILENAME, NULL);
-    g_free (data_dir);
+    *old_dir = get_old_account_data_path (priv);
+    filename = g_build_filename (*old_dir, MC_OLD_AVATAR_FILENAME, NULL);
     return filename;
 }
 
diff --git a/tests/twisted/Makefile.am b/tests/twisted/Makefile.am
index ca3b28db..ac931465 100644
--- a/tests/twisted/Makefile.am
+++ b/tests/twisted/Makefile.am
@@ -226,7 +226,7 @@ BASIC_TESTS_ENVIRONMENT = \
 	XDG_CONFIG_HOME=@abs_top_builddir@/tests/twisted/tmp-$(TMPSUFFIX)/config \
 	XDG_CONFIG_DIRS=@abs_top_srcdir@/tests/twisted \
 	XDG_DATA_HOME=@abs_top_builddir@/tests/twisted/tmp-$(TMPSUFFIX)/localshare \
-	XDG_DATA_DIRS=@abs_top_srcdir@/tests/twisted \
+	XDG_DATA_DIRS=@abs_top_builddir@/tests/twisted/tmp-$(TMPSUFFIX)/share:@abs_top_srcdir@/tests/twisted \
 	XDG_CACHE_DIR=@abs_top_builddir@/tests/twisted/tmp-$(TMPSUFFIX)/cache \
 	XDG_CACHE_HOME=@abs_top_builddir@/tests/twisted/tmp-$(TMPSUFFIX)/cache \
 	MC_CLIENTS_DIR=@abs_top_srcdir@/tests/twisted/telepathy/clients \
diff --git a/tests/twisted/account-manager/avatar-persist.py b/tests/twisted/account-manager/avatar-persist.py
index e9078732..6dfae62a 100644
--- a/tests/twisted/account-manager/avatar-persist.py
+++ b/tests/twisted/account-manager/avatar-persist.py
@@ -27,7 +27,7 @@ import dbus
 import dbus.service
 
 from servicetest import EventPattern, tp_name_prefix, tp_path_prefix, \
-        call_async
+        call_async, assertEquals
 from mctest import exec_test, SimulatedConnection, create_fakecm_account, MC
 import constants as cs
 
@@ -65,8 +65,12 @@ avatar_token=Deus Ex
 """ % account_id)
     accounts_cfg.close()
 
-    os.makedirs(accounts_dir + '/' + account_id)
-    avatar_bin = open(accounts_dir + '/' + account_id + '/avatar.bin', 'w')
+    datadirs = os.environ['XDG_DATA_DIRS'].split(':')
+
+    os.makedirs(datadirs[0] + '/telepathy/mission-control')
+    avatar_filename = (datadirs[0] + '/telepathy/mission-control/' +
+        account_id.replace('/', '-') + '.avatar')
+    avatar_bin = open(avatar_filename, 'w')
     avatar_bin.write('Deus Ex')
     avatar_bin.close()
 
@@ -138,6 +142,44 @@ def test(q, bus, unused):
     assert account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar',
             byte_arrays=True) == conn.avatar
 
+    # The avatar wasn't deleted from $XDG_DATA_DIRS, but it was overridden.
+    assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/' +
+            account_id + '/avatar.bin')
+    assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/fakecm')
+
+    avatar_filename = account_id
+    avatar_filename = avatar_filename.replace('/', '-') + '.avatar'
+    avatar_filename = (os.environ['XDG_DATA_HOME'] +
+        '/telepathy/mission-control/' + avatar_filename)
+    assertEquals('MJ12', ''.join(open(avatar_filename, 'r').readlines()))
+
+    datadirs = os.environ['XDG_DATA_DIRS'].split(':')
+    low_prio_filename = account_id
+    low_prio_filename = low_prio_filename.replace('/', '-') + '.avatar'
+    low_prio_filename = (datadirs[0] +
+        '/telepathy/mission-control/' + low_prio_filename)
+    assertEquals('Deus Ex', ''.join(open(low_prio_filename, 'r').readlines()))
+
+    # If we set the avatar to be empty, that's written out as a file,
+    # so it'll override the one in XDG_DATA_DIRS
+    call_async(q, account_props, 'Set', cs.ACCOUNT_IFACE_AVATAR, 'Avatar',
+            (dbus.ByteArray(''), ''))
+
+    q.expect_many(
+            EventPattern('dbus-method-call',
+                interface=cs.CONN_IFACE_AVATARS, method='ClearAvatar',
+                args=[]),
+            EventPattern('dbus-signal', path=account.object_path,
+                interface=cs.ACCOUNT_IFACE_AVATAR, signal='AvatarChanged'),
+            EventPattern('dbus-return', method='Set')
+            )
+
+    assert account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar',
+            byte_arrays=True) == ('', '')
+
+    assertEquals('', ''.join(open(avatar_filename, 'r').readlines()))
+    assertEquals('Deus Ex', ''.join(open(low_prio_filename, 'r').readlines()))
+
 if __name__ == '__main__':
     preseed()
     exec_test(test, {}, preload_mc=False)
diff --git a/tests/twisted/account-manager/avatar-refresh.py b/tests/twisted/account-manager/avatar-refresh.py
index 42dd5d3f..f3e535a9 100644
--- a/tests/twisted/account-manager/avatar-refresh.py
+++ b/tests/twisted/account-manager/avatar-refresh.py
@@ -27,7 +27,7 @@ import dbus
 import dbus.service
 
 from servicetest import EventPattern, tp_name_prefix, tp_path_prefix, \
-        call_async
+        call_async, assertEquals
 from mctest import exec_test, SimulatedConnection, create_fakecm_account, MC
 import constants as cs
 
@@ -119,6 +119,16 @@ def test(q, bus, unused):
                 ),
             )
 
+    # The avatar got migrated, too.
+    assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/' +
+            account_id + '/avatar.bin')
+    assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/fakecm')
+    avatar_filename = account_id
+    avatar_filename = avatar_filename.replace('/', '-') + '.avatar'
+    avatar_filename = (os.environ['XDG_DATA_HOME'] +
+        '/telepathy/mission-control/' + avatar_filename)
+    assertEquals('Deus Ex', ''.join(open(avatar_filename, 'r').readlines()))
+
 if __name__ == '__main__':
     preseed()
     exec_test(test, {}, preload_mc=False)
diff --git a/tests/twisted/account-manager/avatar.py b/tests/twisted/account-manager/avatar.py
index c616e146..af89c6fc 100644
--- a/tests/twisted/account-manager/avatar.py
+++ b/tests/twisted/account-manager/avatar.py
@@ -16,7 +16,8 @@
 # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
 # 02110-1301 USA
 
-import dbus
+import os
+
 import dbus
 import dbus.service
 
@@ -33,6 +34,13 @@ def test(q, bus, mc):
     account_iface = dbus.Interface(account, cs.ACCOUNT)
     account_props = dbus.Interface(account, cs.PROPERTIES_IFACE)
 
+    assertEquals(cs.ACCOUNT_PATH_PREFIX,
+            account.object_path[:len(cs.ACCOUNT_PATH_PREFIX)])
+    avatar_filename = account.object_path[len(cs.ACCOUNT_PATH_PREFIX):]
+    avatar_filename = avatar_filename.replace('/', '-') + '.avatar'
+    avatar_filename = (os.environ['XDG_DATA_HOME'] +
+        '/telepathy/mission-control/' + avatar_filename)
+
     call_async(q, account_props, 'Set', cs.ACCOUNT_IFACE_AVATAR, 'Avatar',
             dbus.Struct((dbus.ByteArray('AAAA'), 'image/jpeg')))
     q.expect_many(
@@ -46,6 +54,10 @@ def test(q, bus, mc):
     assert account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar',
             byte_arrays=True) == ('AAAA', 'image/jpeg')
 
+    assertEquals('AAAA', ''.join(open(avatar_filename, 'r').readlines()))
+    # We aren't storing in the old location
+    assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/fakecm')
+
     # OK, let's go online. The avatar is set regardless of the CM
     conn, e = enable_fakecm_account(q, bus, mc, account, params,
             has_avatars=True, avatars_persist=True,
@@ -72,6 +84,9 @@ def test(q, bus, mc):
     assert account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar',
             byte_arrays=True) == ('BBBB', 'image/png')
 
+    assertEquals('BBBB', ''.join(open(avatar_filename, 'r').readlines()))
+    assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/fakecm')
+
     someone_else = conn.ensure_handle(cs.HT_CONTACT, 'alberto@example.com')
 
     # Another contact changes their avatar: ignored
@@ -97,6 +112,8 @@ def test(q, bus, mc):
     assert account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar',
             byte_arrays=True) == ('CCCC', 'image/svg')
 
+    assertEquals('CCCC', ''.join(open(avatar_filename, 'r').readlines()))
+
     # empty avatar tests
     conn.forget_avatar()
     q.dbus_emit(conn.object_path, cs.CONN_IFACE_AVATARS, 'AvatarUpdated',
@@ -108,5 +125,9 @@ def test(q, bus, mc):
     assertEquals(account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar',
                                    byte_arrays=False), ([], ''))
 
+    # empty avatars are represented by an empty file, not no file,
+    # to get the right precedence over XDG_DATA_DIRS
+    assertEquals('', ''.join(open(avatar_filename, 'r').readlines()))
+
 if __name__ == '__main__':
     exec_test(test, {})