GNOME Bugzilla – Bug 679918
libsecret migration
Last modified: 2013-03-05 06:33:27 UTC
Created attachment 218801 [details] [review] WIP patch to migrate to libsecret libsecret is a new client for the Secret Service DBus API. The Secret Service allows storage of passwords in a common way on the desktop. Supported by gnome-keyring and ksecretservice. It solves many problems with libgnome-keyring. Relevant to epiphany: it solves threading issues, uses GDBus instead of dbus-glib, and uses GAsyncResult. Note that libsecret can read passwords stored via libgnome-keyring and vs. versa. A future GNOME goal will be to migrate away from libgnome-keyring to libsecret: https://live.gnome.org/GnomeGoals/LibsecretMigration I've done a rough WIP patch for epiphany in order to make sure that the libsecret API covered all the use cases. I'll attach that patch here. I hope it's a help for the migration, but I don't plan to iterate on it at the current time. Some notes about the patch: * Epiphany uses the 'network password' schema, and shares that schema with a bunch of other applications. Those applications put different values in the 'server' fields, so it's not actually able to share passwords. * However when clearing data epiphany clears all passwords that have the 'network password' schema. This would break other applications. * I haven't tested the patch. That's hard for me to do without the devices involved, and I'm not familiar with all the code paths and logic. * Please look at the patch critically, there may be memory leaks or other logic errors. Note that the patch uses the unstable 'advanced' parts of the libsecret API. I'm aiming to get most of this stable by GNOME 3.8, but if you do migrate to libsecret before then, I would patch Epiphany for any API changes that come up.
Created attachment 222169 [details] [review] ephy-embed-single: Add EphySoupPasswordManager and use it Import SoupPasswordManagerGNOME as EphySoupPasswordManager, and use that instead.
per bug 594377, this starts epiphany on the path of not using SoupPasswordManager any more, by at least putting all the keyring/secret-manipulating code into epiphany itself. It still depends on the SoupPasswordManager base class and related SoupAuth methods, but this should move things a little toward being able to merge this stuff with the forms-based-auth stuff...
Thanks both for the patches. ATM I don't have a system with libsecret, so it's hard to test this, but I'll try to get one going as soon as I can (yay jhbuilding).
Are there plans to finish this for 3.6, or is this rather 3.8 stuff? (Setting 3.6 target to be in line with other libsecret migration tickets, but as it's parallel installable we're fine with bumping it to 3.8).
(In reply to comment #4) > Are there plans to finish this for 3.6, or is this rather 3.8 stuff? > (Setting 3.6 target to be in line with other libsecret migration tickets, but > as it's parallel installable we're fine with bumping it to 3.8). Definitely 3.8.
any update on this ?
WebKit has already switched to libsecret, so only only the pre-filled forms need to be ported. I can port it for WebKit2 when I implement pre-filled forms.
Created attachment 236745 [details] [review] ephy-profile-utils: migrate ephy_profile_utils_store/query_form_auth_data() to libsecret We add a new SecretSchema that is specific to epiphany and intended solely to store passwords for webforms. This is a better approach than hacking the server url in order to store the names of the forms in it. These methods are only used by EphyWebView to store the passwords and to retrieve the password when there is a cache match and by one of the early stages of password migration in the profile-migrator. If only this patch is applied, it is likely that only newly saved patchs will work properly, but others will remain intact.
Created attachment 236746 [details] [review] pdm-dialog: migrate to libsecret This is a one-to-one migration from the gkm implementation to libsecret. The existing gkm-powered dialog showed both network authentication and form authentication data. Since we are moving all form authentication data to a Epiphany specific schema, this will not be shown here anymore.
Created attachment 236747 [details] [review] ephy-embed-single: cache form data using libsecret Not only migrate this to libsecret, but also cache the data from the EPHY_FORM_PASSWORD_SCHEMA schema instead of using the GKR-compatibility one. Since we haven't migrated yet the passwords from GKR secrets to the new EPHY_FORM_PASSWORD_SCHEMA schema, the cache will be empty for now.
Created attachment 236748 [details] [review] ephy-profile-migrator: migrate to libsecret Use libsecret to store http-authentication data in one of the early migrators.
Created attachment 236749 [details] [review] ephy-profile-migrator: migrate form passwords to new schema This completes the migration, by moving all passwords previously stored as network passwords to the epiphany form passwords specific schema. It must be noted that some of these passwords were not properly stored as network passwords but as generic passwords, so a throghout search was necessary in order to find all of them.
Created attachment 236750 [details] [review] ephy-password-info: remove as it is unused now
A summary for those not interested in reading the whole story. I added a Epiphany specific schema, org.epiphany.FormPassword, to store properly the information related to a password in a web form. Previously, these were stored as network passwords abusing them in a very hacky way. This schema has all the required fields for form passwords (that is uri, username, and name of the username and password forms), to avoid any need for hacks. With a new migrator step, we migrate all form password data that was previously stored as network passwords to this new schema, getting rid of the (ab)use of the network passwords. For other network passwords, we continue to use the compatibility schema that Stef mentions above. This is suboptimal as this is shared with other applications. WebKit is also using this schema, for what is Bug 694107 needs to be fixed first in order for the migration code to actually work. I attached a patch there and hope to get it in soon.
Review of attachment 236745 [details] [review]: This looks sensible to me. ::: embed/ephy-web-view.c @@ -539,3 @@ - /* FIXME: We use only the first result, for now; We need to do - * something smarter here */ Looking at the rest of the code this is still the case, so we should keep the FIXMEs. ::: lib/ephy-profile-utils.c @@ +215,3 @@ + return FALSE; + + return TRUE; Nitpick: return !g_simple_async ... ?
Review of attachment 236746 [details] [review]: As we talked, this should just show all the auth data (both http and forms). Looks generally sane to me otherwise. ::: src/pdm-dialog.c @@ +6,3 @@ * Copyright © 2009 Igalia S.L. * Copyright © 2009 Holger Hans Peter Freyther + * Copyright © 2013 Igalia S.L. There's an Igalia (c) line above, just add -2013. @@ +1192,3 @@ + gtk_list_store_set (GTK_LIST_STORE (model), &iter, + COL_PASSWORDS_PASSWORD, secret_value_get (secret, NULL), + -1); So I guess here we need a FIXME about copying the secret data into the list store?
Review of attachment 236747 [details] [review]: Nanana.
Review of attachment 236748 [details] [review]: Simple enough.
Review of attachment 236749 [details] [review]: ok desu. ::: lib/ephy-profile-migrator.c @@ +926,3 @@ + + service = secret_service_get_sync (SECRET_SERVICE_OPEN_SESSION | SECRET_SERVICE_LOAD_COLLECTIONS, NULL, NULL); + g_assert (service); The migrator should probably fail gracefully instead of asserting no? This is not a test.
Review of attachment 236750 [details] [review]: Wooooooooo.
Review of attachment 236745 [details] [review]: ::: lib/ephy-profile-utils.h @@ +60,3 @@ + const char *form_username, + const char *form_password, + EphyQueryFormDataCallback callback, Why are you using GAsyncReadyCallback + finish in store, but a custom callback in query?
(In reply to comment #21) > Review of attachment 236745 [details] [review]: > > ::: lib/ephy-profile-utils.h > @@ +60,3 @@ > + const char *form_username, > + const char *form_password, > + EphyQueryFormDataCallback callback, > > Why are you using GAsyncReadyCallback + finish in store, but a custom callback > in query? Not to copy the password around. Of course if you think this is overly cautious I can just use a GAsyncReadyCallback.
Review of attachment 236746 [details] [review]: ::: src/pdm-dialog.c @@ +1192,3 @@ + gtk_list_store_set (GTK_LIST_STORE (model), &iter, + COL_PASSWORDS_PASSWORD, secret_value_get (secret, NULL), + -1); I'm not so sure about this. Not copying this data into the store in any form would imply querying the service every time we need to render. That doesn't seem to be a escalable solution. I think that the current approach of only fetching the passwords until we decide to show them is actually making sense. Perhaps that was the original intention.
Attachment 236745 [details] pushed as 1f50ff4 - ephy-profile-utils: migrate ephy_profile_utils_store/query_form_auth_data() to libsecret Attachment 236746 [details] pushed as 63952a6 - pdm-dialog: migrate to libsecret Attachment 236747 [details] pushed as 44bece7 - ephy-embed-single: cache form data using libsecret Attachment 236748 [details] pushed as e6f502e - ephy-profile-migrator: migrate to libsecret Attachment 236749 [details] pushed as 3d0bd8a - ephy-profile-migrator: migrate form passwords to new schema Attachment 236750 [details] pushed as bce597e - ephy-password-info: remove as it is unused now