GNOME Bugzilla – Bug 704744
"Remember the credential" does not work
Last modified: 2013-07-23 19:56:11 UTC
User can notice that connecting to a remote computer and checking "Remember the credential" will not work since every time he connected he will be asked for the password. Steps : 1. Boot 2. Set a password on a remote computer and enable screen sharing (settings - sharing - screen sharing) 3. Open Vinagre on client computer 4. Connect to the remote computer 5. When asked for password, type it and check "Remember the credential" 6. After successful connection disconnect 7. Reconnect again 8. Observe you are asked for the password again Expected outcome If "Remember the credential" checked password is not required anymore Actual outcome Checking "Remember the credential" does nothing, password asked on every connect
fix pushed in master and gnome-3-8 commit 29ca921de684d402195e72f5dfc0b7bdc4c86cb7 Author: Jonh Wendell <jonh.wendell@intel.com> Date: Tue Jul 23 10:49:24 2013 -0300 Fixes the libsecret 'user' argument passing Use a HashTable to pass arguments to libsecret, so that we can conditionally pass the 'user' key/value. This fixes the bug where cretentials were not being saved/lookup correctly. https://bugzilla.gnome.org/show_bug.cgi?id=704744
Jonh, you are not the maintainer of Vinagre, so in the future I would appreciate it if you asked permission before pushing changes to master and the stable branch. I had no opportunity to review this patch, and I would have had some review comments.I have reverted the patch on master and gnome-3-8 (as well as some other patches on gnome-3-8). I will review the patch on this bug later today.
Created attachment 249928 [details] [review] initial patch
Review of attachment 249928 [details] [review]: The commit message seems to only use 50 characters per line. but I guess it should be using 72. Also, there is a typo: "cretentials" should be "credentials". ::: vinagre/vinagre-tab.c @@ +744,3 @@ +static GHashTable * +create_secret_hash (VinagreTab *tab) I would rather not use a hash table for this, as the strings are duplicated when adding them to the table. I think it is fine to have an if around each secret_password_*v_sync() call, and pass in the "user" attribute only if it is non-NULL.
Created attachment 249934 [details] [review] improved patch I would rather see a patch something like the proposed one here, which I think is simpler, and has the benefit of avoiding creating and destroying the hash table and strings. It probably needs a comment before each use, explaining that the "user" attribute has to go last for it to work.
My first thought was doing this, but I prefer the hash table approach because I wouldn't want to rely on it having to be the last argument, to pretend it's the last. anyway, I'm happy with any alternative, as long as it fixes this bug. feel free to commit your patch instead.
Comment on attachment 249934 [details] [review] improved patch I committed a patch with some added comments, and pushed the result to master and gnome-3-8 as commits 7b301aee94c6c6e62ef5afc0c3be88735f03ba0e and 4474561f619928a5319c36660dbd4ad70dd6503e.