477917 - expose login GUIDs through login manager API

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Bug 467463 added GUIDs to the logins stored in signons.sqlite, but didn't expose them in the login manger API. At the time, we didn't think Weave would actually need to see the GUIDs though the API (it would bang the DB directly), and let us sidestep some complications of adding a "guid" property to nsILoginInfo.

Weave is currently implementing password sync, and the lack if this in the API is causing implementation headaches.

The original concern with adding nsILoginInfo.guid is that it could (1) break existing code and (2) wasn't clear how the .matches() and .equals() methods should deal with it... Would callers need to know the GUID to make these work? Would we need to add wildcard hacks?

I think we can avoid those problems by introducing a new interface, nsILoginMetaInfo, that can hold a login's metainfo. Callers won't need to know about it, but can optionally use it to get or set info on a login. Properties in this interface will be ignored for the purposes of .matches() and .equals().

Need to add some more tests for the added functionality, but the existing tests pass.

Comment 1

[Justin Dolske [:Dolske]]

Also meant to add: this approach will come in useful for bug 465636, so we can expose timestamps and usage counts for a login, without similar concerns about breaking current code or complicating .matches()/.equals().

Comment 2

[Anant Narayanan [:anant]]

Methods that will be of help, if added to the LoginManager:

  getByGUID(GUID) - Get a LoginInfo object by GUID
  removeByGUID(GUID) - Removes the LoginInfo object given a GUID (currently we have to supply all the fields of the record!)

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this late-compat? Can we get someone to scan through AMO to see how many add-ons might need to update themselves if we change this API? Also, someone should contact RoboForm, I bet it'll break 'em.

Comment 4

[Dan Mills [:thunder]]

Since Justin's patch uses a new interface, there should be zero compat impact.  Users of the current API continue as-is.  Those who want to find out the GUID of a login do a QI to a new interface and get it that way.

Comment 5

[Dan Mills [:thunder]]

I forgot to add:

If we do implement getByGUID/removeByGUID as Anant requested, there should be minimal compat impact, since we're only adding to the interface.  But anyone who depends on the login manager uuid (because they're replacing it or something) might be affected.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

OK, we like Weave enough to block. Aren't they lucky!

Comment 7

[Justin Dolske [:Dolske]]

Yeah, I don't think this patch creates any significant late-compat concerns. Adding a getByGUID() interface would cause the interface UUID to be revved, which would be a minor issue for binary code, but otherwise has no impact.

There's one potential edge-caseish way existing code could break, though. If someone takes a login returned from login manager and reuses the object for something else, the nsILoginMetaInfo attached to it could cause unexpected results.

EG:

var logins = pwmgr.getAllLogins();
var recycledLogin = logins[0];
// reinit everything
recycledLogin.hostname = "foo"
recycledLogin.username = "bar";
recycledLogin.password = "baz";
// ...etc...
pwmgr.addLogin(recycledLogin);

That would try to add a new login with the same GUID, which will now fail. [Well, when I update the patch it will.] A similar problem could be encountered with modifyLogin().

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

This changes modifyLogin() to allow having the second argument be either a nsILoginInfo or a nsIPropertyBag2.

If it's a nsILoginInfo, things work as they do today, and nsILoginMetaInfo properties on the old login are unchanged.

If it's a nsIPropertyBag2, the specified properties will be changed on the old login. One can change nsILoginMetaInfo properties this way.

At one point gavin had me thinking we could do without the nsIPropertyBag stuff, and just always have modifyLogin() update the old login's nsILoginMetaInfo property values with the new login. But I ran into some compatibility issues:

  * Callers would need to make sure the old login was a real login obtained from login manager (not hand-built) or else the metainfo would get reset to defaults. Existing code in login manager would have needed changed, so it's likely extensions would need changed too.
  * Potential ambiguity if we add boolean properties to nsILoginMetaInfo in the future... There's no way to say "use the existing value from the old login" (as, say, one could do with null/empty-string for string properties).

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 363001 [details] [diff] [review]
Patch v.2

>diff --git a/toolkit/components/passwordmgr/public/nsILoginMetaInfo.idl b/toolkit/components/passwordmgr/public/nsILoginMetaInfo.idl

>+/**
>+ * An object containing metainfo for a login stored by the login manager.

>+ * compating logins, thes properties are ignored.

"comparing logins, these"

>+interface nsILoginMetaInfo : nsISupports {

>+    attribute AString guid;

I wonder whether we should ensure that this is non-empty/null in the setter to avoid people screwing things up... I was going to suggest making it readonly, but I guess the internal code relies on being able to set it.

>+#define NS_LOGINMETAINFO_CONTRACTID "@mozilla.org/login-manager/loginMetaInfo;1"

This isn't needed, right? Isn't even hooked up...

>diff --git a/toolkit/components/passwordmgr/src/storage-Legacy.js b/toolkit/components/passwordmgr/src/storage-Legacy.js

>     modifyLogin : function (oldLogin, newLogin) {
>+        if (newLogin instanceof Ci.nsIPropertyBag2)
>+            throw "legacy modifyLogin with propertybag not implemented.";

throw Components.Exception("foo", Components.results.NS_ERROR_NOT_IMPLEMENTED); is friendlier to C++ callers, I think, and ensures the source file is noted in the error console. I guess this file throws raw strings already, perhaps we should change them all seperately, though.

>+    modifyLogin : function (oldLogin, newLoginData) {

>+                        newLogin[prop.name] = newLoginData.getPropertyAsAString(prop.name);
>+                        break;

>+                    case "guid":
>+                        newLogin.guid = newLoginData.getPropertyAsAString("guid");

Can't you just use prop.value? xpconnect should convert it to a string automatically.

>      * Returns the |id| for the specified login, or null if the login was not
>      * found.

Update this comment?

Comment 10

[Justin Dolske [:Dolske]]

(In reply to comment #9)

> >+    attribute AString guid;
> 
> I wonder whether we should ensure that this is non-empty/null in the setter to
> avoid people screwing things up... I was going to suggest making it readonly,
> but I guess the internal code relies on being able to set it.

A null/empty value can be used to indicate addLogin() should generate a GUID automatically. It's not looked at elsewhere in the API. The nsILoginMetaInfo values are ignored with modifyLogin and removeLogin, so a null/empty value  won't matter there either.

> >+#define NS_LOGINMETAINFO_CONTRACTID "@mozilla.org/login-manager/loginMetaInfo;1"
> 
> This isn't needed, right? Isn't even hooked up...

Oops. Yeah, this was just cut'n'paste from nsILoginInfo.idl.

> throw Components.Exception("foo", Components.results.NS_ERROR_NOT_IMPLEMENTED);
> is friendlier to C++ callers, I think, and ensures the source file is noted in
> the error console. I guess this file throws raw strings already, perhaps we
> should change them all seperately, though.

Bug 394686! :)

> >+                        newLogin[prop.name] = newLoginData.getPropertyAsAString(prop.name);
> >+                        break;
>
> Can't you just use prop.value? xpconnect should convert it to a string
> automatically.

Oh, hey, I can. I forget about the magic nsIVariant converstion. In fact, this argument doesn't need to be a nsIPropertyBag2, and can just be a nsIPropertyBag [no "2"].

Comment 11

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/cfc8878b0eac

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> A null/empty value can be used to indicate addLogin() should generate a GUID
> automatically.

Yeah, I know, but I'm saying that it doesn't make sense for it to be possible to set it to null/"" after it's been given a real GUID, and my concern was that if we somehow ended up doing a _isGuidUnique(null) for whatever reason, we could get into trouble. But maybe that isn't a problem in practice.

Comment 13

[Justin Dolske [:Dolske]]

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/84b722f9b668

Comment 14

[Eric Shepherd [:sheppy]]

The docs are updated.