Bug 39796 - Can't remove secret params from the keyfile
Summary: Can't remove secret params from the keyfile
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: mission-control (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-03 03:50 UTC by Guillaume Desmottes
Modified: 2011-08-31 03:21 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Don't flag a parameter has secret if we are about to remove it (855 bytes, patch)
2011-08-03 03:56 UTC, Guillaume Desmottes
Details | Splinter Review
account-manager-default: always delete secret keys from both locations (1.29 KB, patch)
2011-08-03 05:30 UTC, Guillaume Desmottes
Details | Splinter Review
Avoid a warning when trying to store a NULL value (1.75 KB, patch)
2011-08-03 05:30 UTC, Guillaume Desmottes
Details | Splinter Review

Description Guillaume Desmottes 2011-08-03 03:50:10 UTC
- Open your accounts.cfg and add "param-password=badger" to an account
- Start MC
- Call UpdateParameters() on this Account to unset the password
- Call GetAll(): the param has been removed
- Restart MC
- Call GetAll(): the param is back!

That's because the param is not removed from the key file.

Patch coming.
Comment 1 Guillaume Desmottes 2011-08-03 03:56:10 UTC
Created attachment 49869 [details] [review]
Don't flag a parameter has secret if we are about to remove it

This leads the parameter has not actually being removed in the key file
backend.
Comment 2 Simon McVittie 2011-08-03 04:10:59 UTC
12:07 < smcv> cassidy: hmm, wouldn't it be better if this bit in 
              mcd-a-m-default.c <http://paste.debian.net/124947/> removed it 
              from whichever of secrets and keyfile it's not setting it in?
12:07 < smcv> cassidy: and for bonus points cope with NULL gracefully, like the 
              !ENABLE_GNOME_KEYRING case does
12:08 < smcv> probably easiest to do: remove_key (secrets, ...); remove_key 
              (keyfile, ...); if (val != NUL) { if (secret) set(secrets, ...) 
              else set(keyfile, ...) }
12:09 < smcv> cassidy: also, do you get a critical or warning when you 
              reproduce that bug, symptomatic of calling set(..., NULL)?
Comment 3 Guillaume Desmottes 2011-08-03 05:10:55 UTC
(In reply to comment #2)
> 12:07 < smcv> cassidy: hmm, wouldn't it be better if this bit in 
>               mcd-a-m-default.c <http://paste.debian.net/124947/> removed it 
>               from whichever of secrets and keyfile it's not setting it in?
> 12:07 < smcv> cassidy: and for bonus points cope with NULL gracefully, like the 
>               !ENABLE_GNOME_KEYRING case does
> 12:08 < smcv> probably easiest to do: remove_key (secrets, ...); remove_key 
>               (keyfile, ...); if (val != NUL) { if (secret) set(secrets, ...) 
>               else set(keyfile, ...) }
> 12:09 < smcv> cassidy: also, do you get a critical or warning when you 
>               reproduce that bug, symptomatic of calling set(..., NULL)?


_set() is not called in this case, _delete() is, so that won't solve this
issue.
Comment 4 Simon McVittie 2011-08-03 05:16:45 UTC
(In reply to comment #3)
> _set() is not called in this case, _delete() is

Remove it from both locations in the implementation of _delete(), then? Pseudo-patch:

 #if ENABLE_GNOME_KEYRING
-      if (mcp_account_manager_parameter_is_secret (am, account, key))
-        save = g_key_file_remove_key (amd->secrets, account, key, NULL);
-      else
-        save = g_key_file_remove_key (amd->keyfile, account, key, NULL);
+      save = g_key_file_remove_key (amd->keyfile, account, key, NULL);
+      save = g_key_file_remove_key (amd->secrets, account, key, NULL) || save;
Comment 5 Guillaume Desmottes 2011-08-03 05:30:20 UTC
Created attachment 49872 [details] [review]
account-manager-default: always delete secret keys from both locations

This ensures that the key is actually removed even it has been tagged as
secret after is removal.
Comment 6 Guillaume Desmottes 2011-08-03 05:30:25 UTC
Created attachment 49873 [details] [review]
Avoid a warning when trying to store a NULL value

e17b83fec8f01ca6977cb4fe259a83634aa90b26 only fixed the !ENABLE_GNOME_KEYRING
case.
Comment 7 Guillaume Desmottes 2011-08-04 05:11:17 UTC
Merged to master; will be 5.9.1.
Comment 8 Will Thompson 2011-08-31 03:21:45 UTC
(In reply to comment #7)
> Merged to master; will be 5.9.1.

(Actually, it will be in 5.9.2; it was not in 5.9.1 due to a branch mix-up.)


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.