Comment on attachment 713377
Store Master Password by using libsecret to Gnome Keyring patch v1
Review of attachment 713377:
-----------------------------------------------------------------
Overall looks good. Haven't tested. Just a few comments.
::: configure.in
@@ +4989,5 @@
> +
> + if test "$MOZ_ENABLE_LIBSECRET"
> + then
> + PKG_CHECK_MODULES(MOZ_LIBSECRET, libsecret-1 >= $LIBSECRET_VERSION,[
> + MOZ_LIBSECRET_LIBS=`echo $MOZ_LIBSECRET_LIBS | sed
> 's/-llinc\>//'`
Are you sure you need this? If so, I'd be interested to know on which
platform libsecret has that in its pkg-config file.
::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +751,5 @@
> +on_lookup_finished(GObject *source,
> + GAsyncResult *result,
> + gpointer data)
> +{
> + gchar *pwd = secret_password_lookup_finish(result, nullptr);
Is it worth retrieving the error and logging it if things fail? May make
our lives easier in the future if people run into problems and we have
to diagnose them remotely.
@@ +803,5 @@
> + nsCString keyDescription;
> + keyDescription.Assign(MOZ_APP_NAME);
> + keyDescription.Append(" - ");
> + keyDescription.Append(profileName);
> + mRunning = true;
May I suggest using (the equivalent of):
MOZ_APP_NAME + " Master Password - " + profileName
So that it's clear in the password manager what this is?
@@ +808,5 @@
> +
> + secret_password_lookup(MOZILLA_SECRET_SCHEMA, nullptr, on_lookup_finished,
> this,
> + "application", MOZ_APP_NAME,
> + "profile", profileName.get(),
> + NULL);
Are you sure you need to do this as async? I don't understand all
aspects of how SyncRunnableBase works, but if this is a separate thread,
you could just use secret_password_lookup_sync().
::: security/manager/ssl/src/nsPK11TokenDB.cpp
@@ +388,5 @@
> + };
> + return &the_schema;
> +}
> +
> +#define MOZILLA_SECRET_SCHEMA get_mozilla_secret_schema()
Is there a reason the above function and macro can't be shared between
nsNSSCallbacks.cpp and nsPK11TokenDB.cpp?
@@ +400,5 @@
> +on_libsecret_password_stored(GObject *source,
> + GAsyncResult *result,
> + gpointer data)
> +{
> + secret_password_store_finish (result, nullptr);
Is it worth retrieving the error and logging it if things fail? May make
our lives easier in the future if people run into problems and we have
to diagnose them remotely.
@@ +416,5 @@
> +on_libsecret_password_cleared(GObject *source,
> + GAsyncResult *result,
> + gpointer data)
> +{
> + secret_password_clear_finish(result, nullptr);
Ditto about the error.
--
You received this bug notification because you are a member of Desktop
Packages, which is subscribed to firefox in Ubuntu.
https://bugs.launchpad.net/bugs/41179
Title:
Integrate with Gnome Keyring
Status in The Mozilla Firefox Browser:
Confirmed
Status in “firefox” package in Ubuntu:
Won't Fix
Status in “xulrunner-1.9” package in Ubuntu:
Won't Fix
Status in “xulrunner-1.9.1” package in Ubuntu:
Triaged
Bug description:
For a really good Gnome integration, it would be great to have the
ability to save passwords in the Gnome keyring.
A similar thing has been proposed for Epiphany: see
https://launchpad.net/malone/bugs/3467.
To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/41179/+subscriptions
--
Mailing list: https://launchpad.net/~desktop-packages
Post to : desktop-packages@lists.launchpad.net
Unsubscribe : https://launchpad.net/~desktop-packages
More help : https://help.launchpad.net/ListHelp
