Bug 39796

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.

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)?

(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.

(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;

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.

Created attachment 49873 [details] [review]
Avoid a warning when trying to store a NULL value

e17b83fec8f01ca6977cb4fe259a83634aa90b26 only fixed the !ENABLE_GNOME_KEYRING
case.

Merged to master; will be 5.9.1.

(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.)