Bug 42088 - Unsetting a secret password doesn't remove it from the keyring
Unsetting a secret password doesn't remove it from the keyring
Status: RESOLVED FIXED
Product: Telepathy
Classification: Unclassified
Component: mission-control
unspecified
Other All
: medium normal
Assigned To: Simon McVittie
Telepathy bugs list
http://cgit.freedesktop.org/~smcv/tel...
r+
: patch
Depends on:
Blocks: 32578
  Show dependency treegraph
 
Reported: 2011-10-21 02:55 UTC by Guillaume Desmottes
Modified: 2012-09-27 17:25 UTC (History)
1 user (show)

See Also:


Attachments
Make the gnome-keyring test work again, with modern gnome-keyring (2.01 KB, patch)
2012-08-30 16:23 UTC, Simon McVittie
Details | Splinter Review
Default account backend: when deleting, always delete from both places (2.57 KB, patch)
2012-08-30 18:33 UTC, Simon McVittie
Details | Splinter Review
_keyring_commit: perform deletions for keys in removed, not in secrets (1.51 KB, patch)
2012-08-30 18:33 UTC, Simon McVittie
Details | Splinter Review
Default account backend: when deleting from the keyring, remove from secrets (1.17 KB, patch)
2012-08-30 18:34 UTC, Simon McVittie
Details | Splinter Review
Default account backend: when deleting passwords, delete the same thing we will look for (1.59 KB, patch)
2012-08-30 18:34 UTC, Simon McVittie
Details | Splinter Review
account-store-default: load the same names that MC would (1.87 KB, patch)
2012-08-30 18:34 UTC, Simon McVittie
Details | Splinter Review
Test deletion of passwords from accounts (3.86 KB, patch)
2012-08-30 18:35 UTC, Simon McVittie
Details | Splinter Review
Default account backend: test that *all* passwords are deleted (4.55 KB, patch)
2012-08-30 18:35 UTC, Simon McVittie
Details | Splinter Review
Default accounts backend: finish password migrations that Empathy 3.0 started (3.24 KB, patch)
2012-09-17 09:54 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Guillaume Desmottes 2011-10-21 02:55:34 UTC
Once a MC password has been imported by Empathy, it calls UpdateParameters({}, ('password')) to unset it, but this doesn't remove it from gnome-keyring as expected.
Comment 1 Guillaume Desmottes 2011-10-21 05:20:35 UTC
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.
Comment 2 Guillaume Desmottes 2011-10-21 05:30:19 UTC
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',))
Comment 3 Will Thompson 2012-05-23 04:48:17 UTC
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.
Comment 4 Simon McVittie 2012-07-19 09:53:16 UTC
(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...
Comment 5 Simon McVittie 2012-08-30 16:23:30 UTC
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.
Comment 6 Simon McVittie 2012-08-30 18:33:13 UTC
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).
Comment 7 Simon McVittie 2012-08-30 18:33:38 UTC
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.
Comment 8 Simon McVittie 2012-08-30 18:34:00 UTC
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!
Comment 9 Simon McVittie 2012-08-30 18:34:24 UTC
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".
Comment 10 Simon McVittie 2012-08-30 18:34:45 UTC
Created attachment 66363 [details] [review]
account-store-default: load the same names that MC would

This tool was previously not deleting "param-".
Comment 11 Simon McVittie 2012-08-30 18:35:16 UTC
Created attachment 66364 [details] [review]
Test deletion of passwords from accounts
Comment 12 Simon McVittie 2012-08-30 18:35:37 UTC
Created attachment 66365 [details] [review]
Default account backend: test that *all* passwords are  deleted
Comment 13 Jonny Lamb 2012-08-30 20:06:19 UTC
All your patches are great!
Comment 14 Simon McVittie 2012-08-31 14:30:01 UTC
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.
Comment 15 Simon McVittie 2012-09-13 17:42:42 UTC
(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.
Comment 16 Simon McVittie 2012-09-17 09:54:34 UTC
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.
Comment 17 Simon McVittie 2012-09-17 10:01:45 UTC
http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=512-finish-migrating for those who prefer to review in cgit.
Comment 18 Paul Menzel 2012-09-17 15:09:54 UTC
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.
Comment 19 Paul Menzel 2012-09-17 15:11:41 UTC
This bug is tracked as #687933 in the Debian BTS [1].

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?archive=no&bug=687933
Comment 20 Simon McVittie 2012-09-17 15:25:27 UTC
(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).
Comment 21 Guillaume Desmottes 2012-09-20 11:11:49 UTC
++
Comment 22 Simon McVittie 2012-09-27 17:25:58 UTC
Fixed in 5.12.3 and 5.13.2.