GNOME Bugzilla – Bug 579217
Switch pdm-dialog.c to use GnomeKeyring for passwords
Last modified: 2013-12-11 11:47:31 UTC
Please describe the problem: Remove the Epiphany Password Manager class from embed and replace it with direct calls to gnome-keyring. It might make more sense to remove this tab completely and go with seahorse for password management as it is quite tricky to avoid the password being copied to pagable memory. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 132790 [details] [review] Move pdm-dialog.c to GnomeKeyring Move things to GnomeKeyring. There are three kind of issues: - The implementation should be switched to async gkr calls - The current remove implementation will remove the row even if "Deny" is clicked. So the gnome-keyring library returns _OK and not _DENIED - The secret gets strdup'ed in gnome-keyring and when calling g_object_set on the cell renderer, this should be avoided.
(In reply to comment #1) > Created an attachment (id=132790) [edit] > Move pdm-dialog.c to GnomeKeyring > > Move things to GnomeKeyring. There are three kind of issues: > - The implementation should be switched to async gkr calls > - The current remove implementation will remove the row even if "Deny" is > clicked. So the gnome-keyring library returns _OK and not _DENIED > - The secret gets strdup'ed in gnome-keyring and when calling g_object_set > on the cell renderer, this should be avoided. > One big thing: This will get the passwords from the main keyring in all cases, but for private instances we don't want to do that. We are also missing the other part, in private instances we need to tell webkit to not store the passwords in the persistent keyring, but in a temporary one (which would be the one to be used to show the data in the PDM dialog). This alone could actually be a good reason why we can't use seahorse in general, right? Some comments: + EphyPasswordInfo *other = g_new (EphyPasswordInfo, 1); + if (other == NULL) + return NULL; glib/gtk+ programs in general won't be able to recover when memory allocation is failing, so we don't check this. +EphyPasswordInfo +*ephy_password_info_new (guint32 key_id) The '*' goes in the previous line. + EphyPasswordInfo *info = g_new0 (EphyPasswordInfo, 1); + if (info == NULL) + return NULL; Same than first comment. + gtk_tree_model_get_value (model, &iter, + COL_PASSWORDS_PASSWORD, &val); + result = pdm_dialog_password_remove(pinfo, + g_value_get_boxed (&val)); The indentation is wrong here. + * Remove the item. For cookies we from callback, for passwords + * we remove right here. + */ Missing one 'remove' there. + /* FIXME: get_secret makes insecure copy... */ + if (result == GNOME_KEYRING_RESULT_OK) { + info->secret = gnome_keyring_memory_strdup ( + gnome_keyring_item_info_get_secret (kinfo)); + /* eek, this will do a strdup in GtkCellRendererText... */ + g_object_set (cell, "text", info->secret, NULL); + g_value_unset(&val); Any idea how to fix these? The first one isn't an issue in gnome keyring itself? + guint key_id = GPOINTER_TO_UINT(data); Missing space. And as you say, should move to the async versions of the functions.
(In reply to comment #2) > (In reply to comment #1) > > Created an attachment (id=132790) [edit] > > Move pdm-dialog.c to GnomeKeyring > > > > Move things to GnomeKeyring. There are three kind of issues: > > - The implementation should be switched to async gkr calls > > - The current remove implementation will remove the row even if "Deny" is > > clicked. So the gnome-keyring library returns _OK and not _DENIED > > - The secret gets strdup'ed in gnome-keyring and when calling g_object_set > > on the cell renderer, this should be avoided. > > > > One big thing: > > This will get the passwords from the main keyring in all cases, but for private > instances we don't want to do that. We are also missing the other part, in > private instances we need to tell webkit to not store the passwords in the > persistent keyring, but in a temporary one (which would be the one to be used > to show the data in the PDM dialog). > > This alone could actually be a good reason why we can't use seahorse in > general, right? Yes, this would be a good reason to have that in epiphany. > + /* FIXME: get_secret makes insecure copy... */ > + if (result == GNOME_KEYRING_RESULT_OK) { > + info->secret = gnome_keyring_memory_strdup ( > + > gnome_keyring_item_info_get_secret (kinfo)); > > + /* eek, this will do a strdup in GtkCellRendererText... */ > + g_object_set (cell, "text", info->secret, NULL); > + g_value_unset(&val); > > Any idea how to fix these? The first one isn't an issue in gnome keyring > itself? Yes, I will need to file a bug against gnome keyring and see if I'm missing something.
Created attachment 138988 [details] [review] Epiphany-gnome-keyring.patch Updated patch that addresses most of the coding style issues and also updates password UI a bit. Things in TODO list (in the order of importance): * Handle "Show Passwords" checkbox * Multiple passwords for one resource (currently seems to select last pass) * Use async calls * Don't copy passwords into mappable * Fix the tabs vs space issues and coding style;)
Created attachment 139166 [details] [review] 0001-Set-password-column-initially-as-hidden.patch Takes care of the "Handle 'Show Passwords' checkbox" issue
Created attachment 139167 [details] [review] 0002-Use-correct-column-from-store-for-deletion.patch Makes the "Delete all" functionality work
Things in TODO: * Use async calls * Fix the tabs vs space issues and coding style;) * Don't copy passwords into mappable (possibly gnome-keyring issue) Things that belong in WebKit: * Multiple passwords for one resource (password storing works, just selection fails)
Created attachment 139289 [details] [review] 0003-Make-pdm_dialog_fill_passwords_list-asynchronous.patch One down, 4 sync calls remaining...
Created attachment 139312 [details] [review] 0004-Make-password-loading-showing-asynchronous.patch 3 sync calls to go...
Created attachment 139315 [details] [review] 0004-Make-password-loading-showing-asynchronous.patch Forgotten g_unset_val() in one place...
Created attachment 139367 [details] [review] 0005-Make-clear-all-functionality-asynchronous.patch Still need to figure how to properly free the data in the PASSWORDS_DATA column :S
Created attachment 139411 [details] [review] 0006-Password-removal-is-now-async.patch
Created attachment 139455 [details] [review] 0007-Password-adding-is-fully-async-now.patch
I have pushed my changes (after a small fiasco) to gnome-keyring branch in git.gnome.org. Feature-wise it is mostly complete (except we need to show warning when Webkit/libsoup has no keyring stuff enabled), but this requires upstream support first.
Some more comments on top of the ones I gave on IRC: - gpointer data); - void (* remove) (PdmActionInfo *info, - gpointer data); + gpointer data); + void (* remove) (PdmActionInfo *info, + gpointer data); void (* scroll_to) (PdmActionInfo *info); Broken indentation. +typedef struct PdmCallBackData PdmCallBackData; +struct PdmCallBackData +{ + guint key; + GtkListStore *store; }; Mmmm, this does not give any warning? typedef struct PdmCallbackData { ... }; is enough. - COL_PASSWORDS_PASSWORD, - COL_PASSWORDS_DATA + COL_PASSWORDS_PASS, + COL_PASSWORDS_DATA, + PASSWORDS_NUM_COL }; Why? + if (path != NULL && gtk_tree_model_get_iter (model, &iter, path)) { + /* FIXME! Do we have to drop the data too? */ + gtk_list_store_remove (GTK_LIST_STORE (model), &iter); + + gtk_tree_path_free (path); + } I believe that should be released when the row is removed too. +static gboolean +clear_all_passwords (GtkTreeModel *model, + GtkTreePath *path, + GtkTreeIter *iter, + gpointer data) +{ + GtkTreeRowReference *row; + EphyPasswordInfo *info; + GValue val = { 0, }; + + row = gtk_tree_row_reference_new (model, path); + + gtk_tree_model_get_value (model, iter, + COL_PASSWORDS_DATA, &val); + info = g_value_get_boxed (&val); + + gnome_keyring_item_delete (GNOME_KEYRING_DEFAULT, + info->keyring_id, + (GnomeKeyringOperationDoneCallback) pdm_dialog_password_remove_cb, + row, + (GDestroyNotify) gtk_tree_row_reference_free); + g_value_unset (&val); + return FALSE; +} This clears one password, not all of them, so it should be renamed. Also, any reason in particular to use gtk_tree_model_get_value? (Just wondering) + PdmDialog *pdialog = EPHY_PDM_DIALOG (checkbuttons->dialog); + PdmActionInfo *pinfo = pdialog->priv->passwords; + + gtk_tree_model_foreach (pinfo->model, (GtkTreeModelForeachFunc) clear_all_passwords, NULL); Indentation is wrong. Also, although in this case I think it will work because we are doing it in an async way, removing rows while iterating with _foreach is wrong. I think I prefer to keep the construct Holger had here (the while loop). gtk_dialog_set_response_sensitive (GTK_DIALOG (dialog), GTK_RESPONSE_OK, - data->num_checked != 0); + data->num_checked != 0); } + void pdm_dialog_show_clear_all_dialog (EphyDialog *edialog, - GtkWidget *parent, - PdmClearAllDialogFlags flags) + GtkWidget *parent, + PdmClearAllDialogFlags flags) There's lots of these kind of indentation breakage around. /* Removal */ - for (r = rlist; r != NULL; r = r->next) - { - GValue val = { 0, }; - - path = gtk_tree_row_reference_get_path - ((GtkTreeRowReference *)r->data); - - gtk_tree_model_get_iter (model, &iter, path); - gtk_tree_model_get_value (model, &iter, action->data_col, &val); - action->remove (action, g_value_get_boxed (&val)); - g_value_unset (&val); - - /* for cookies we delete from callback, for passwords right here */ - if (action->delete_row_on_remove) - { - gtk_list_store_remove (GTK_LIST_STORE (model), &iter); - } - - gtk_tree_row_reference_free ((GtkTreeRowReference *)r->data); - gtk_tree_path_free (path); - } + action->remove (action, (GtkTreeRowReference *)r->data); Mmm, why did you need to change all this code? Wasn't it enough to change the remove method for passwords to use the async gkr function? +static void +passwords_data_func_get_item_cb (GnomeKeyringResult result, + GnomeKeyringItemInfo *info, + gpointer data) +{ + GtkTreeRowReference *rowref = (GtkTreeRowReference *)data; + + if (result == GNOME_KEYRING_RESULT_OK) { + GtkTreeIter iter; + GtkTreePath *path; + GtkTreeModel *model; + + if (!gtk_tree_row_reference_valid (rowref)) + return; + + path = gtk_tree_row_reference_get_path (rowref); + model = gtk_tree_row_reference_get_model (rowref); + + if (path != NULL && gtk_tree_model_get_iter (model, &iter, path)) { + EphyPasswordInfo *epinfo; + GValue val = {0, }; + + gtk_tree_model_get_value (model, &iter, COL_PASSWORDS_DATA, &val); + epinfo = g_value_get_boxed (&val); + epinfo->secret = gnome_keyring_memory_strdup (gnome_keyring_item_info_get_secret (info)); + + gtk_list_store_set (GTK_LIST_STORE (model), &iter, + COL_PASSWORDS_DATA, epinfo, + COL_PASSWORDS_PASS, epinfo->secret, -1); + g_value_unset (&val); + } + } Most of this function seems to be repeated 3 times or so, I think it would be worth factoring it out if we still repeat it so much after finishing the patch. +static void +pdm_dialog_password_add (PdmActionInfo *info, + gpointer data) +{ + PdmCallBackData *cbdata; + guint key_id = GPOINTER_TO_UINT(data); + + /* + * We have the item id of the password. We will have to check if this + * password entry is of the right type and then can proceed to get the + * the private information. Seahorse is treating every protocol that + * starts with http as Web Access and we will do the same here. + */ + + cbdata = g_malloc (sizeof(PdmCallBackData *)); You should allocate the size of the struct, not the size of a pointer to the struct. I'm not sure how this is not crashing. And in any case, use g_slice_new.
I have rebased the gnome-keyring branch and fixed most of the things you pointed out. Although there are some things that I couldn't figure out: > This clears one password, not all of them, so it should be renamed. Also, any > reason in particular to use gtk_tree_model_get_value? (Just wondering) I don't understand the gtk_tree_model_get_value() part :( > Indentation is wrong. Also, although in this case I think it will work because > we are doing it in an async way, removing rows while iterating with _foreach is > wrong. I think I prefer to keep the construct Holger had here (the while loop). The _foreach thing still there.. (Need to read treeview/model stuff a bit more.. )
(In reply to comment #16) > I have rebased the gnome-keyring branch and fixed most of the things you > pointed out. Although there are some things that I couldn't figure out: > > > This clears one password, not all of them, so it should be renamed. Also, any > > reason in particular to use gtk_tree_model_get_value? (Just wondering) > > I don't understand the gtk_tree_model_get_value() part :( Well, I *think* that generally when coding in C is much more common to use gtk_tree_model_get. > > > Indentation is wrong. Also, although in this case I think it will work because > > we are doing it in an async way, removing rows while iterating with _foreach is > > wrong. I think I prefer to keep the construct Holger had here (the while loop). > > The _foreach thing still there.. (Need to read treeview/model stuff a bit > more.. ) http://scentric.net/tutorial/sec-treemodel-remove-many-rows.html
I pushed the branch as gnome-keyring-v2 (In reply to comment #17) > (In reply to comment #16) > > I don't understand the gtk_tree_model_get_value() part :( > > Well, I *think* that generally when coding in C is much more common to use > gtk_tree_model_get. Done (at least in this place) > > > > > Indentation is wrong. Also, although in this case I think it will work because > > > we are doing it in an async way, removing rows while iterating with _foreach is > > > wrong. I think I prefer to keep the construct Holger had here (the while loop). > > > > The _foreach thing still there.. (Need to read treeview/model stuff a bit > > more.. ) > > http://scentric.net/tutorial/sec-treemodel-remove-many-rows.html > I used the Holger's construct..
I don't see any of my comments addressed in those branches. I've checked it out and fixed some things, but the branch seems to completely break removing cookies...
OK, I've pushed this, with my fixes, to master. I'm leaving this open for a few nitpicks: - The view redraws itself annoyingly when removing cookies (possible passwords too, but you can't see it because you don't tend to have so many). - We need to figure out the keyring-using-unsafe-memory thing. - If we don't move the keyring stuff out of WebKit and into libsoup-gnome we'll need some kind of warning if keyring support isn't enabled.