GNOME Bugzilla – Bug 753355
Store credentials using secrets service
Last modified: 2015-11-23 11:18:45 UTC
Created attachment 308900 [details] [review] Store credentials for RDP Attached patch adds support for storing of credentials using secrets service to RDP plugin. The patch handles unsuccessful authentications by asking user for credentials. It also adds optional "domain" attribute to credentials handling and adds ability to show default values for 'username' and 'domain' in authentication dialog.
Review of attachment 308900 [details] [review]: Looks good, but can you split the domain changes into a separate patch? Thanks!
Created attachment 311379 [details] [review] Handle domain when looking for credentials Here is the first part which adds the domain property and its usage in the base code.
Created attachment 311380 [details] [review] Store credentials for RDP This is the second part which enables the credentials storage for RDP plugin.
Review of attachment 311379 [details] [review]: Generally looks fine. Does this work fine with existing passwords, which will not have the domain attribute? ::: vinagre/vinagre-tab.c @@ +754,3 @@ + *domain = NULL; + + /* "domain" goes last, to terminate the attribute list if conn_domain is NULL. */ This (probably) breaks storing of passwords where the username is NULL, as well as the domain. See commit 7b301aee94c6c6e62ef5afc0c3be88735f03ba0e for the full reasoning. If both domain and username can be NULL, you need to take care that the parameter list passed to secret_passwork_lookup_sync() is terminated with a NULL. @@ +821,3 @@ if (tab->priv->saved_credentials) { + /* Put "domain" last, to terminate the attributes if conn_domain is NULL. */ Same comment as above.
Review of attachment 311380 [details] [review]: Looks fine, although this will have to wait until the freeze is over, and the domain patch has been merged.
(In reply to David King from comment #4) > Review of attachment 311379 [details] [review] [review]: > > Generally looks fine. Does this work fine with existing passwords, which > will not have the domain attribute? Yes, it works for me when no domain is specified. > ::: vinagre/vinagre-tab.c > @@ +754,3 @@ > + *domain = NULL; > + > + /* "domain" goes last, to terminate the attribute list if conn_domain is > NULL. */ > > This (probably) breaks storing of passwords where the username is NULL, as > well as the domain. See commit 7b301aee94c6c6e62ef5afc0c3be88735f03ba0e for > the full reasoning. If both domain and username can be NULL, you need to > take care that the parameter list passed to secret_passwork_lookup_sync() is > terminated with a NULL. I actually see a problem when username is NULL and domain is not NULL only. I didn't suppose that it could happen but I can modify the code so that it wouldn't cause troubles. If username and domain are NULL, then the list of parameters passed to the secret_passwork_lookup_sync() would end with 5 NULLs which I thought is OK. > @@ +821,3 @@ > if (tab->priv->saved_credentials) > { > + /* Put "domain" last, to terminate the attributes if conn_domain is > NULL. */ > > Same comment as above.
Created attachment 315982 [details] [review] Handle domain when looking for credentials I've rewritten the calls of secret functions to use GHashTable for passed attributes. This way we don't need to handle the terminal NULLs.
Review of attachment 315982 [details] [review]: Looks fine to me.
Comment on attachment 311380 [details] [review] Store credentials for RDP Thank you for the reviews. I've pushed both patches to master.