It would be nice to store configuration data in g_get_user_config_dir() instead of a dotfile.
(In reply to comment #0) > It would be nice to store configuration data in g_get_user_config_dir() instead > of a dotfile. Currently you can change the default using --with-accounts-dir. If we are going to get rid of ~/.mission-control, then do we plan to have some sort of a migration mechanism?
(In reply to comment #1) > If we are going to get rid of ~/.mission-control, then do we plan to have some > sort of a migration mechanism? That'd be a prerequisite. If we want to improve the storage format (one keyfile per account, maybe?), doing that at the same time would be nice, so we only need one migration. I'm not entirely sure whether Telepathy accounts are configuration (g_get_user_config_dir, usually ~/.config) or application data (g_get_user_data_dir, usually ~/.local/share); various people have opined that keeping them in GConf (as Mission Control 4 did) was an abuse of GConf, but perhaps GConf and ~/.config aren't considered to be the same thing?
user name and password are definitely user data not configuration data: 1. The user is entering information (=data) about himself. He is not changing anything in the behavior of the program. 2. Entering login data (account data) does not change the intended (default) way of using the program, it just allows the user to use the full functionality. 3. If the user wishes to wipe his .config directory to reset the program to default settings he does not want to loose his saved password or user name. 4. Configuration settings are customizations that let the program deviate from the intended default usage scenario. However entering login data is a default usage scenario. Based on these arguments i would argue to put user name and password in XDG_DATA_DIR
*** Bug 38253 has been marked as a duplicate of this bug. ***
(In reply to comment #1) > If we are going to get rid of ~/.mission-control, then do we plan to have some > sort of a migration mechanism? That's the thing blocking this, really: changing the default is trivial, but losing everyone's accounts in the process isn't acceptable. While we're migrating anyway, this would be a good time to consider a better accounts-file format: would one GKeyFile per account be better? On the other hand, if we add a plugin to use an as-yet-hypothetical GNOME online accounts service (or for that matter, KWallet?), that makes this bug relatively irrelevant.
Ping. This would be really great to get fixed. We're trying to fix all GNOME apps. https://live.gnome.org/GnomeGoals/XDGConfigFolders
Created attachment 60593 [details] [review] Migrate accounts to use XDG config directory
(In reply to comment #5) > While we're migrating anyway, this would be a good time to consider a better > accounts-file format: would one GKeyFile per account be better? I don't really want to do a migration/upgrade step without also introducing a better on-disk format. This comment is relevant: /* FIXME: this is ridiculous. MC stores the parameters for the account, so * it should be able to expose them on D-Bus even if the CM is uninstalled. * It shouldn't need to iterate across the parameters supported by the CM. * But it does, because MC doesn't store the types of parameters. So it * needs the CM (or .manager file) to be around to tell it whether "true" * is a string or a boolean… We should store one account per file in a GVariant-based format, probably.
Might be worth coordinating with Evolution. They are just introducing a new accounts file format. IIRC, using keyfiles.
Any update on this ?
(In reply to comment #10) > Any update on this ? Yes I'm working on it right now.
Oh, I worked on this ages ago but forgot to update this bug when I'd done the branch. I just updated it to master. Simon, what do you think of this as a first go?
Would be nice to get this in, it would make the xdg dirs support for GNOME 3.6 more complete.
I've only reviewed the first couple of commits so far. ---- > + (GDestroyNotify) g_free, (GDestroyNotify) g_hash_table_unref); Nitpicking: g_free is already a GDestroyNotify and doesn't need this cast. > +/* simply replaces '/' with '_' */ > +static gchar * > +escape_path (const gchar *str) This function can be reduced to return g_strdelimit (g_strdup (str), "/", '_'); To guarantee collision-free filenames I think I'd prefer either '-' (or anything in [+%,=~^@] if you prefer), or keeping '/' (leading to use of subdirectories), rather than '_', yielding filenames like this: ~/.config/mission-control/accounts/gabble-jabber-smcv_40example_2ecom (_ is special because we're using tp_escape_as_identifier, so it's the escape character.) > + /* TODO: use this instead of MC_ACCOUNT_DIR > + self->directory = g_build_filename ( > + g_get_user_config_dir (), "mission-control", "accounts", NULL); > + */ Presumably fixed later > + /* TODO: this will only work in the test environment for now. */ > + self->directory = g_build_filename (g_getenv ("MC_ACCOUNT_DIR"), NULL); Presumably fixed later > + props = g_hash_table_new_full (g_str_hash, g_str_equal, > + (GDestroyNotify) g_free, (GDestroyNotify) tp_g_value_slice_free); Why not string => GVariant? (I suppose you have to do the same sort of serialization dance in commit_one either way, though.) > The big problem with this right now is that the mcp API depends > on all account properties being strings. Once the API is fixed > we can update this plugin easily to support it, and actually save > types to disk! I'd prefer to add get_variant(), set_variant() vfuncs (but I see you did that later so never mind). > +/* We happen to know that the string MC gave us is "sufficiently escaped" to > + * put it in the keyfile as-is. */ > +static gboolean > +_set (const McpAccountStorage *storage, This comment is irrelevant copypasta. > + mcp_account_manager_set_value (am, account, > + key, val); It's not your fault, but this API is madness... > +static gboolean > +_delete (const McpAccountStorage *storage, > + const McpAccountManager *am, > + const gchar *account, > + const gchar *key) key is ignored, the entire account is deleted. Is this how it's meant to work? > + /* owned (gchar *) of account => (gpointer) boolean whether account > + * has changed and so needs to be written to disk. in reality the > + * value is always GUINT_TO_POINTER(TRUE) or the account is simply > + * absent. */ > + GHashTable *accounts_modified; It's (becoming) conventional to represent a set as a hash table (key => key). GLib 2.32 has some magic to make those a bit more space-efficient, even. See the GHashTable docs.
(In reply to comment #14) > > + /* owned (gchar *) of account => (gpointer) boolean whether account > > + * has changed and so needs to be written to disk. in reality the > > + * value is always GUINT_TO_POINTER(TRUE) or the account is simply > > + * absent. */ > > + GHashTable *accounts_modified; > > It's (becoming) conventional to represent a set as a hash table (key => key). > GLib 2.32 has some magic to make those a bit more space-efficient, even. See > the GHashTable docs. FWIW, you don't even have to know the key->key trick, just use g_hash_table_add/contains()
> account-storage: add a fallback for a missing set_variant vfunc I don't think this commit is right - it doesn't account for escaping? I would expect set_variant() to take a GVariant, whereas set() gets the escaped-for-a-key-file equivalent as per _storage_set_value() in plugin-account.c (which you could factor out into a GVariant-to-string function). For instance, if some attribute is a TAB character (a string of length 1), then set_variant() should see g_variant_new_string ("\t"), whereas set() should see "\\t" (a string of length 2) because that's what you have to write in your GKeyFile. String lists in keyfiles are separated by ";", not "," as in this commit, and if a list element contains ";" it has to be escaped to "\;". Same for object path lists. To go the other way, see _storage_dup_value() and telepathy-glib/util.c. GLib itself has g_key_file_get_[u]int64 these days, too. > keyfile storage: store everything as strings, not values > Is this going to break everything? I hope not. Yes, this is probably going to break the GOA and UOA plugins I'm afraid. We should keep the "escape it"-based API for set() and get(), and do it Right™ for set_variant() and get_variant(). Alternatively, break plugin ABI and bump the SONAME. > plugin-account: stop storing everything in a keyfile in memory TODO: not reviewed in detail since it seems to rely on the above change. I approve of this in principle, but if we're not breaking the plugin ABI, we should keep the escaping. > +'AutomaticPresenceType': <2>, Isn't this meant to be a uint32 in GVariant-land? > rval = g_file_set_contents (a->filename, str, -1, NULL); > + _mcd_chmod_private (a->filename); If the file wasn't already restricted, its contents can be stolen by a very fast attacker - g_file_set_contents() respects the umask, unfortunately. Either chmod the parent directory before writing into it, or have MC force its umask to 077 on startup so all files it creates are private-by-default. Actually, now I say that, I don't know why we didn't use the umask in the first place - it would clearly be correct. Oh yeah, it was because MC used to be some bizarre daemon/library hybrid, but thankfully that's no longer the case... > + priv->account_connections_dir = g_build_filename (g_get_user_cache_dir (), > + "mission-control", "accounts", NULL); I'd prefer this to end up in g_get_user_cache_dir()/telepathy/mc_connections (i.e. not a dotfile, and share a directory with tp-glib and the logger). > +/* man I can't wait until this file can be deleted */ > +static void > +move_everything_else (void) I'm not convinced that .mc_connections needs migrating at all, I think deletion would be sufficient - that's why it's in a cache directory. The only time migration would be useful is if you upgrade from < this version to >= this version, and then crash/kill MC (without ending your desktop session) with at least one account connected. If you do care enough to migrate it, just move it as a special case, rather than looping over a speculatively general "everything else". > default keyring storage test: skip for now I'd be tempted to delete the default backend and just rely on migration. If not, we should at least delete the gnome-keyring bits. > + * This file is part of account_migrator-control Ahem. > + /* TODO */ > + g_file_set_contents (filename, > + "# Accounts have been migrated", -1, NULL); I would prefer less data loss: when an account is migrated fully, delete its group from the GKeyFile; if one cannot be migrated (usually because the CM wasn't installed any more), leave it in in the hope that we can do better later. > +mcp_account_storage_set_variant (const McpAccountStorage *storage, > + const McpAccountManager *am, Const pointers to GObjects are pretty counterproductive (you can't even ref them). > +GVariant * > +mcp_account_manager_get_variant (const McpAccountManager *mcpa, > + const gchar *account, > + const gchar *key) > +{ ... > + g_return_val_if_fail (iface->set_variant != NULL, NULL); Should say "get". > tmp = escape_path (account); > path = g_build_filename (self->directory, tmp, NULL); > - g_hash_table_insert (self->accounts_to_paths, > - g_strdup (account), path); > + > + a = account_new (self, account, path); > + > + g_free (path); > g_free (tmp); If you made account_new() calculate tmp and path, that'd save a malloc/free cycle. No big deal though. (In reply to comment #14) > > +_delete (const McpAccountStorage *storage, > key is ignored, the entire account is deleted. Is this how it's meant to work? Fixed later anyway. > It's (becoming) conventional to represent a set as a hash table (key => key). Fixed later anyway.
There is one more thing which should be migrated but you don't yet, and that's the avatar, which is kept in (for instance) ~/.mission-control/accounts/gabble/jabber/smcv_40example_2ecom0/avatar.bin. I would suggest moving it to $XDG/mission-control/accounts/avatars/smcv_40example_2ecom0.bin or something, for some value of $XDG. (In reply to comment #0) > It would be nice to store configuration data in g_get_user_config_dir() instead > of a dotfile. I still think the existence of a MC account is more like "user data" (~/.local/share) than it's like "configuration" (~/.config), as in Comment #2 and Comment #3. On the other hand, many of the parameters we store in accounts are clearly more like configuration than credentials, so I'm willing to be overruled on this. Where does Evolution 3.6 put email accounts? I notice GOA uses ~/.config. The XDG spec for these directories says "user-specific configuration files" and "user-specific data files" without really defining what those mean, so that's no help. https://live.gnome.org/GnomeGoals/XDGConfigFolders says "If you remove the .config folder, the user will see all the preferences reset to the default but without any data loss". I'm not sure that keeping accounts in .config is consistent with this: if I delete my configuration, I would expect (for instance) the Empathy theme, privacy settings, etc. to reset, but maybe not my list of IM accounts?
(In reply to comment #17) > There is one more thing which should be migrated but you don't yet, and that's > the avatar, which is kept in (for instance) > ~/.mission-control/accounts/gabble/jabber/smcv_40example_2ecom0/avatar.bin. I > would suggest moving it to > $XDG/mission-control/accounts/avatars/smcv_40example_2ecom0.bin or something, > for some value of $XDG. This should have said $XDG/mission-control/accounts/gabble-jabber-smcv_40example_2ecom0.bin.
(In reply to comment #18) > This should have said > $XDG/mission-control/accounts/gabble-jabber-smcv_40example_2ecom0.bin. ... or even better, something with "avatar" in it too. *more coffee*
(In reply to comment #16) > If the file wasn't already restricted, its contents can be stolen by a very > fast attacker - g_file_set_contents() respects the umask, unfortunately. McdMaster already sets a private umask, so this is not a problem. > Either chmod the parent directory before writing into it, or [...] MC already sets umask. chmod is only necessary on platforms that have chmod but not umask, or for files that could have existed before we used umask (MC 5.2.2 or platforms without umask). Since only MC >= 5.13.x will ever have the new files added in this branch, and umask is required by POSIX, my inclination is to rely on umask and not bother with the (inherently race-prone unless you are really careful) chmod. We might want to add: #ifdef G_OS_UNIX # ifndef HAVE_UMASK # error MC relies on umask() for privacy on Unix systems # endif #endif just to make sure? Windows security is completely different anyway, and g_chmod() is ineffective there, so, whatever. I hope we can assume that CSIDL_LOCAL_APPDATA is private.
(In reply to comment #5) > While we're migrating anyway, this would be a good time to consider a better > accounts-file format: would one GKeyFile per account be better? I think we should do this: it's not difficult. (In reply to comment #8) > We should store one account per file in a GVariant-based format, probably. This, on the other hand, is a massive unshaven yak. Migrating from a format with less information to a format with more information turns out to be quite difficult... so I'm tempted to just migrate ~/.mission-control/accounts/accounts.cfg to $XDG_DATA_DIRS/.local/share/telepathy/accounts/${ACCOUNT_NAME}.ini (one keyfile per account) and do the GVariantization later. The account-storage plugin API is also a mess, and fixing that involves GAsync'ing everything, which, again, would be great to do, but is a pretty big yak to shave. (In reply to comment #16) > > + priv->account_connections_dir = g_build_filename (g_get_user_cache_dir (), > > + "mission-control", "accounts", NULL); > > I'd prefer this to end up in g_get_user_cache_dir()/telepathy/mc_connections > (i.e. not a dotfile, and share a directory with tp-glib and the logger). If you don't use --with-accounts-cache-dir or $MC_ACCOUNT_DIR, which you generally shouldn't, then this file is already ${XDG_CACHE_HOME}/.mc_connections. So, no migration needed. Yay. (In reply to comment #17) > the avatar, which is kept in (for instance) > ~/.mission-control/accounts/gabble/jabber/smcv_40example_2ecom0/avatar.bin. We can move these, sure.
Created attachment 66611 [details] [review] [1/3] Tests: distinguish between XDG cache/config/data home, and MC_ACCOUNT_DIR We want to be sure that MC is using the right one in each situation. --- This patch, together with two more that I'm about to attach, migrates avatars to, for instance, ~/.local/share/telepathy/mission-control/gabble-jabber/smcv_40example_2ecom0.avatar. Avatars are considerably easier than the rest of the account because they don't have an excessively complicated plugin API. I'm still looking into unbonging the account storage API.
Created attachment 66612 [details] [review] [2/3] mcd_ensure_directory: add I keep calling g_mkdir_with_parents() and expecting it to return a boolean (it doesn't, it returns 0 or -1), so it seems worth wrapping it in something more sensible.
Created attachment 66613 [details] [review] [3/3] 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.
Created attachment 66788 [details] [review] Default account backend: restructure to be able to look in multiple places This isolates all file-writing into _commit, and all file-reading into am_default_load_keyfile.
Created attachment 66789 [details] [review] Default account backend: include directory creation in _commit Also check for errors - we can at least give a useful warning.
Created attachment 66790 [details] [review] Default account backend: write accounts to XDG_DATA_HOME, with fallback If the user has ~/.mission-control/accounts/accounts.cfg we migrate from there to XDG_DATA_HOME/telepathy/mission-control/accounts.cfg, and if successful, delete the old name. If the user has XDG_DATA_DIRS/telepathy/mission-control/accounts.cfg (in a lower-priority path element than XDG_DATA_HOME), we use it, with copy-on-write into XDG_DATA_HOME. (Limitation: the account-store executable used in some tests only reads from XDG_DATA_HOME, and doesn't understand the XDG_DATA_DIRS and MC_ACCOUNT_DIR fallback.)
Created attachment 66791 [details] [review] Update documentation of --with-accounts-dir It used to be where we kept accounts; now it's only the source for account migration into XDG_DATA_HOME. Keep its name the same, so that if a user or distribution is configuring with --with-accounts-dir="~/.mc-accounts" or something, the migration works as intended as long as they continue to do so. (We don't recommend doing so, though.)
(In reply to comment #3) > user name and password are definitely user data not configuration data I agree, so I used the data directories. Perhaps one day we'll split out "configuration" parameters (DisplayName, AutomaticPresence, require-tls etc.) from "data" parameters (the existence of the account, its login name, the server name you need to hard-code otherwise it doesn't work on your network, the current avatar), but that can wait until we've shaved the "make account storage make sense" yak. I think doing it right would require a TP_CONN_MGR_PARAM_FLAG_CONFIG analogous to ..._SECRET. (In reply to comment #21) > I'm tempted to just migrate > ~/.mission-control/accounts/accounts.cfg to > $XDG_DATA_DIRS/.local/share/telepathy/accounts/${ACCOUNT_NAME}.ini (one keyfile > per account) and do the GVariantization later. This turns out to be just as complex as GVariant'ing everything, because our internal storage model is poorly suited to splitting up into individual keyfiles. I just kept the "one big keyfile called accounts.cfg" model.
Comment on attachment 66613 [details] [review] [3/3] McdAccount: migrate avatars to a XDG path Review of attachment 66613 [details] [review]: ----------------------------------------------------------------- ::: tests/twisted/account-manager/avatar-persist.py @@ +150,5 @@ > + 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())) Why do you do ''.join(open('filename')).readlines()) and not just open('filename').read()?
Okay all your patches look fine. I got a bit bogged down with some of your test changes but they're probably fine. Hothothothothtohtohtohtot.
(In reply to comment #30) > Why do you do ''.join(open('filename')).readlines()) and not just > open('filename').read()? Because I'd forgotten how convenient Python is, compared with C; I thought that'd sometimes read an arbitrary prefix of the file rather than the whole thing. I'll append a patch.
(In reply to comment #21) > (In reply to comment #5) > > While we're migrating anyway, this would be a good time to consider a better > > accounts-file format: would one GKeyFile per account be better? > > I think we should do this: it's not difficult. [citation needed] I actually never got here... > (In reply to comment #8) > > We should store one account per file in a GVariant-based format, probably. > > This, on the other hand, is a massive unshaven yak. Considerable yak-shaving later, Bug #54870, Bug #54872, Bug #54873, Bug #54780 (*), Bug #54874 and Bug #54875 (*) implement this. Branches marked (*) involve a storage format or content transition, so you can't safely downgrade MC past the version that introduced them. If we proceed directly to Bug #54875 (62 commits later) and never make a release that has this branch merged but Bug #54875 unmerged, we can skip directly from ~/.mission-control/accounts/accounts.cfg to the GVariant-based files I wanted all along, and not have to have a migration route via XDG_DATA_DIRS/telepathy/mission-control/accounts.cfg. This would allow deletion of maybe 50-100 lines of code. On the other hand, the hard bits of backwards compatibility are all necessary anyway, which is why the saving isn't actually very large.
> If we proceed directly to Bug #54875 (62 commits later) and never make a > release that has this branch merged but Bug #54875 unmerged, we can skip > directly from ~/.mission-control/accounts/accounts.cfg to the GVariant-based > files I wanted all along This isn't going to happen for 5.14 if we want it to happen at roughly the same time as GNOME 3.4. Jonny and I agreed IRL that we should land the branches from this bug (which he already reviewed) and get them in 5.14, then do the transition to GVariants in 5.15. So: bug as-stated fixed in git for 5.13.2 (whose release-prep I'm doing now), and see Bug #54875 and its blockers for the other stuff.
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.