After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 579217 - Switch pdm-dialog.c to use GnomeKeyring for passwords
Switch pdm-dialog.c to use GnomeKeyring for passwords
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Passwords, Cookies, & Certificates
git master
Other All
: Normal normal
: ---
Assigned To: Priit Laes (IRC: plaes)
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-04-16 20:42 UTC by Holger Hans Peter Freyther
Modified: 2013-12-11 11:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Move pdm-dialog.c to GnomeKeyring (35.08 KB, patch)
2009-04-16 20:46 UTC, Holger Hans Peter Freyther
none Details | Review
Epiphany-gnome-keyring.patch (45.74 KB, patch)
2009-07-22 11:49 UTC, Priit Laes (IRC: plaes)
none Details | Review
0001-Set-password-column-initially-as-hidden.patch (1.37 KB, patch)
2009-07-24 16:37 UTC, Priit Laes (IRC: plaes)
none Details | Review
0002-Use-correct-column-from-store-for-deletion.patch (790 bytes, patch)
2009-07-24 16:38 UTC, Priit Laes (IRC: plaes)
none Details | Review
0003-Make-pdm_dialog_fill_passwords_list-asynchronous.patch (2.02 KB, patch)
2009-07-27 15:37 UTC, Priit Laes (IRC: plaes)
none Details | Review
0004-Make-password-loading-showing-asynchronous.patch (5.30 KB, patch)
2009-07-27 19:46 UTC, Priit Laes (IRC: plaes)
none Details | Review
0004-Make-password-loading-showing-asynchronous.patch (5.33 KB, patch)
2009-07-27 19:50 UTC, Priit Laes (IRC: plaes)
none Details | Review
0005-Make-clear-all-functionality-asynchronous.patch (3.22 KB, patch)
2009-07-28 10:24 UTC, Priit Laes (IRC: plaes)
none Details | Review
0006-Password-removal-is-now-async.patch (5.86 KB, patch)
2009-07-28 19:04 UTC, Priit Laes (IRC: plaes)
none Details | Review
0007-Password-adding-is-fully-async-now.patch (6.25 KB, patch)
2009-07-29 09:38 UTC, Priit Laes (IRC: plaes)
none Details | Review

Description Holger Hans Peter Freyther 2009-04-16 20:42:54 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:
Comment 1 Holger Hans Peter Freyther 2009-04-16 20:46:17 UTC
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.
Comment 2 Xan Lopez 2009-04-24 07:10:01 UTC
(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.

Comment 3 Holger Hans Peter Freyther 2009-05-05 05:14:09 UTC
(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.
Comment 4 Priit Laes (IRC: plaes) 2009-07-22 11:49:14 UTC
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;)
Comment 5 Priit Laes (IRC: plaes) 2009-07-24 16:37:45 UTC
Created attachment 139166 [details] [review]
0001-Set-password-column-initially-as-hidden.patch

Takes care of the "Handle 'Show Passwords' checkbox" issue
Comment 6 Priit Laes (IRC: plaes) 2009-07-24 16:38:36 UTC
Created attachment 139167 [details] [review]
0002-Use-correct-column-from-store-for-deletion.patch

Makes the "Delete all" functionality work
Comment 7 Priit Laes (IRC: plaes) 2009-07-24 16:39:59 UTC
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)
Comment 8 Priit Laes (IRC: plaes) 2009-07-27 15:37:28 UTC
Created attachment 139289 [details] [review]
0003-Make-pdm_dialog_fill_passwords_list-asynchronous.patch

One down, 4 sync calls remaining...
Comment 9 Priit Laes (IRC: plaes) 2009-07-27 19:46:58 UTC
Created attachment 139312 [details] [review]
0004-Make-password-loading-showing-asynchronous.patch

3 sync calls to go...
Comment 10 Priit Laes (IRC: plaes) 2009-07-27 19:50:23 UTC
Created attachment 139315 [details] [review]
0004-Make-password-loading-showing-asynchronous.patch

Forgotten g_unset_val() in one place...
Comment 11 Priit Laes (IRC: plaes) 2009-07-28 10:24:55 UTC
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
Comment 12 Priit Laes (IRC: plaes) 2009-07-28 19:04:09 UTC
Created attachment 139411 [details] [review]
0006-Password-removal-is-now-async.patch
Comment 13 Priit Laes (IRC: plaes) 2009-07-29 09:38:20 UTC
Created attachment 139455 [details] [review]
0007-Password-adding-is-fully-async-now.patch
Comment 14 Priit Laes (IRC: plaes) 2009-07-29 10:33:19 UTC
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.
Comment 15 Xan Lopez 2009-07-30 14:10:47 UTC
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.
Comment 16 Priit Laes (IRC: plaes) 2009-07-31 16:32:38 UTC
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.. )
Comment 17 Xan Lopez 2009-07-31 19:20:07 UTC
(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 

Comment 18 Priit Laes (IRC: plaes) 2009-08-01 18:16:51 UTC
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..
Comment 19 Xan Lopez 2009-08-08 18:18:13 UTC
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...
Comment 20 Xan Lopez 2009-08-08 18:36:15 UTC
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.