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 Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/217300
Title:
Seahorse integration
To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/217300/+subscriptions
--
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
