Bug 42088

Looks like it's supposed to get removed in mcd-account-manager-default:_keyring_commit() but is not because the key is not in amd->secrets at this point.

I wrote a stupid script to help me debugging this.
(pasting as for some reason bz doesn't me to attach it)


import gobject
import dbus
import dbus.glib

import sys

BUS = 'org.freedesktop.Telepathy.AccountManager'
PATH = '/org/freedesktop/Telepathy/Account/gabble/jabber/cassidy_2dtest1_40jabber_2ebelnet_2ebe0'
IFACE = 'org.freedesktop.Telepathy.Account'

bus = dbus.SessionBus()
iface = dbus.Interface(bus.get_object(BUS, PATH), IFACE)

if sys.argv[1] == 'set':
    print iface.UpdateParameters({'password': 'XXX'}, ())
else:
    print iface.UpdateParameters({}, ('password',))

I can confirm that this issue still exists with MC 1:5.12.0-2, Empathy 3.4.2.1-1 and gnome-keyring 3.4.1-3.

When I log in, two passwords for my Collabora account show up in Seahorse:

• One, labelled “Telepathy password”, is the old, wrong one.
• The other, labelled “Instant messaging password”, is the new, right one.

The former shows up in `mc-tool show`.

Hitting Clear and unticking [ ] Remember password in Empathy removes the new, correct password from the keyring, and stops the old password from appearing in `mc-tool show`. Disabling and re-enabling the account throws up a password prompt. I enter the correct password there, and bingo! I'm online. But this only lasts for this session. If I log out and in again, MC picks the old password back up again.

I guess one fix for this bug would be to remove Gnome Keyring support from MC (bug 32578), but *something* would have to clear up the old password entries.

(In reply to comment #1)
> Looks like it's supposed to get removed in
> mcd-account-manager-default:_keyring_commit() but is not because the key is not
> in amd->secrets at this point.

Anything that's being deleted ought to be deleted from both places (keyfile *and* gnome-keyring), regardless of whether MC thinks it's secret.

Somehow I doubt it's actually that easy once you get into the bowels of MC account storage, but it ought to be possible...

Created attachment 66348 [details] [review]
Make the gnome-keyring test work again, with modern  gnome-keyring

---

This is a prerequisite for having any sort of confidence that we've fixed this.

Created attachment 66359 [details] [review]
Default account backend: when deleting, always delete  from both places

Our tracking of whether something is "secret" (in the gnome-keyring) is
pretty shaky, and in particular, we can forget that things are meant
to be "secret" sometimes (we lose that information when deleting
parameters).

Happily, when we're deleting things, it doesn't actually matter: the
right thing to do is clearly to delete from both locations, regardless
of where we think it ought to be.

Similarly, when we're setting a property to a new value, it's appropriate
to delete it from both locations, then put it in the right location
(only).

Created attachment 66360 [details] [review]
_keyring_commit: perform deletions for keys in removed,  not in secrets

'removed' is essentially a set of (account, key) tuples that should
be deleted. What we were doing was: 

    foreach account in removed
        foreach key in secrets[account]
            delete (account, key)

which makes little sense - if we have param-password and
param-proxy-password and we want to unset Parameters['password'],
the current implementation would delete both. This commit changes it to:

    foreach account in removed
        foreach key in removed[account]
            delete (account, key)

which has the advantage of actually making sense.

Created attachment 66361 [details] [review]
Default account backend: when deleting from the keyring,  remove from secrets

Otherwise we'd just delete it, then (because it's still in secrets)
re-commit it!

Created attachment 66362 [details] [review]
Default account backend: when deleting passwords, delete  the same thing we will look for

Deleting secrets with param="param-password" isn't a whole lot of use
when we save, and look up, param="password".

Created attachment 66363 [details] [review]
account-store-default: load the same names that MC would

This tool was previously not deleting "param-".

Created attachment 66364 [details] [review]
Test deletion of passwords from accounts

Created attachment 66365 [details] [review]
Default account backend: test that *all* passwords are  deleted

All your patches are great!

Fixed in git for 5.13.1.

I'm leaving this open for the time being because this bug broke Empathy's gnome-keyring migration, so we should probably add a cleanup step which deletes any passwords that have a corresponding (post-migration) Empathy version.

Also, I haven't backported to 5.12 yet, but maybe we should consider it once this has had more testing.

(In reply to comment #14)
> Also, I haven't backported to 5.12 yet, but maybe we should consider it once
> this has had more testing.

Backported and released in 5.12.2.

(In reply to comment #14)
> I'm leaving this open for the time being because this bug broke Empathy's
> gnome-keyring migration, so we should probably add a cleanup step which deletes
> any passwords that have a corresponding (post-migration) Empathy version.

Still needed. I have most of a patch, but I still need to test it.

Created attachment 67272 [details] [review]
Default accounts backend: finish password migrations  that Empathy 3.0 started

---

This gets rid of the fallout from this bug for people who upgraded to Empathy 3 while using MC < 5.12.2. I'd like to put it in 5.12.x.

http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=512-finish-migrating for those who prefer to review in cgit.

Comment on attachment 67272 [details] [review]
Default accounts backend: finish password migrations  that Empathy 3.0 started

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

Thanks for the patch. Looks good, although I have not tested it.

::: src/mcd-account-manager-default.c
@@ +320,5 @@
> +
> +                  DEBUG ("An Empathy 3.0 password migration wasn't finished "
> +                      "due to MC bugs. Finishing it now by deleting the "
> +                      "password for %s", account);
> +

Please add the fdo bug number to the debug output.

This bug is tracked as #687933 in the Debian BTS [1].

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?archive=no&bug=687933

(In reply to comment #18)
> Please add the fdo bug number to the debug output.

Good point, I'll replace "MC bugs" with "fd.o #42088". I'm waiting for MC 1:5.12.1-3 to migrate into wheezy, and also for another Telepathy upstream developer to review the patch here; then I'll upload 1:5.12.1-4 to Debian.

For the record, this fd.o bug also represents Debian bug #686836, which is what caused passwords not to be deleted (actually more like three separate bugs, each of which on its own would have had those symptoms).

++

Fixed in 5.12.3 and 5.13.2.