Closed Bug 479894 Opened 15 years ago Closed 15 years ago

Add a property-bag based searchLogins API to login manager

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: Dolske, Assigned: zpao)

References

Details

(Keywords: dev-doc-complete, fixed1.9.1)

Attachments

(2 files, 5 obsolete files)

Spun off from bug 477917 comment 2.

Weave needs the ability to get a login matching a specific GUID. But we should really implement a more generic search API [especially since findLogins() is rather limiting.]

The caller would pass in a nsIPropertyBag2 containing properties the login must match, and then we return an array of matching logins. Weave's use-case is then just a basic usage of this -- Weave would pass in a bag like { guid: "123-456-7890"}.

Paul noted an interesting addition to this -- does nsIPropertyBag allow duplicate property names (with differing value), and could we use that to support searches like "all logins where the username is FOO or BAR"? Seems like that would be useful, but we might want to keep the scope simple/narrow, and not support that initially.
Flags: blocking1.9.1?
Dolske, this seems like a new feature, and it's a bit late in the game to take for 1.9.1, right?  Minusing for now, but interested in hearing why you would hold 1.9.1 for this.
Flags: blocking1.9.1? → blocking1.9.1-
Weave is the driving force for this.
Attached patch Patch v0.1 (WIP) (obsolete) — Splinter Review
So unless(until?) we go forward with my plan to make a mozStorage framework, we need a way to to more arbitrary searches in our password manager storage. This is the WIP solution, and allows us to search using any fields via a public interface.
Paul, can you get this into reviewable form so it can try to make beta 4?
Marking bug 316084 blocking this since this patch will be getting rid of _queryLogins and bug 316084 modifies it first. I'd block the other way if possible, but this bug isn't blocking 3.5.
Depends on: 316084
Attached patch Patch v0.2 (obsolete) — Splinter Review
Built on top of the patch for 316084.

I opted for using property bag to the public method, and a JS object for the internal method. This keeps the internal use simple & allows us to get rid of _queryLogins.
Attachment #364849 - Attachment is obsolete: true
Attachment #368925 - Flags: review?(dolske)
Comment on attachment 368925 [details] [diff] [review]
Patch v0.2

>--- a/toolkit/components/passwordmgr/public/nsILoginManagerStorage.idl
...
>+interface nsIPropertyBag2;

Can the API just use a nsIProperyBag, or must it be a nsIProperyBag2?

>+    void getAllMatchingLogins(

"searchLogins" would be a lot simpler.

>--- a/toolkit/components/passwordmgr/src/storage-mozStorage.js

>-     * Returns an array of nsAccountInfo.
>+     * Returns an array of nsILoginInfo.

What the heck! :)

>+    getAllMatchingLogins : function(count, matchData) {
...
>+        // XXXzpao Should we filter fields we don't want used in public searches?

Eh, I don't think there's much point. Someone who really wants it could just bang the DB with SQL anyway.

>+    /*
>+     * _getAllMatchingLogins
>+     *
>+     * Private method to perform arbitrary searches on any field. Decryption is
>+     * left to the caller.
>+     *
>+     * Returns [logins, ids] for logins that match the arguments, where logins
>+      * is an array of encrypted nsLoginInfo and ids is an array of associated
>+      * ids in the database.
>+     */

Indent is funny here.

>+    _getAllMatchingLogins : function (matchData) {
...
>+        let hostname = formSubmitURL = httpRealm = null;

I think empty-string, not null, is the value you meant here.

But...

I think it would be simpler to not use buildConditionsAndParams() here. Instead, have getAllLogins() pass in an empty matchData, and findLogins() pass in only params which are not empty-string. Then this function can just build up the query more or less as normal, with some minimal extra processing in the swtich for when these params are null (and the fun OR clause for formSubmitURL).

It would be nice if countLogins() could go through this same path (since it's the only other buildConditionsAndParams() caller), but I don't see an easy way to do that without complicating things.

>+        try {
>+            stmt = this._dbCreateStatement(query, params);

We should probably spin off a bug for not caching all these statements, because the query string will differ depending on the fields specified (and the order we happen to pull them out of the propertybag?).

The tricky bit now is that we probably want to cache *some* statements. Say, getAllLogins() should ideally hit a cached statement, but not a random searchLogins(). Maybe only cache the last N statements. Fodder for another bug...

>+++ b/toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_7.js

More numbers, *sadface*. I wonder if we should just call it something like test_mozStorage_api_searchLogins.js, and eventually move other tests towards a scheme like that.

>+testdesc = "checking that getAllMatchingLogins works using nsiPropertyBag"

Nit: nsIPropertyBag. (Capital-I) This is in a few other places in the patch too...

>+matchData.setPropertyAsAString("id", "1");
>+matchData.setPropertyAsAString("hostname", "");
>+matchData.setPropertyAsAString("httpRealm", "");
>+matchData.setPropertyAsAString("formSubmitURL", "");

Should probably have variations here with passing |null|. Cut'n'paste from tests for findLogins()?

Also, test with:
 * empty matchData (same as getAllLogins)
 * bogus matchData property (check for error)
 * search that returns nothing
Attachment #368925 - Flags: review?(dolske) → review-
(In reply to comment #7)
> Can the API just use a nsIProperyBag, or must it be a nsIProperyBag2?

I was using some methods particular to nsIPropertyBag2, but changed that, so nsIPropertyBag should work.

> >-     * Returns an array of nsAccountInfo.
> >+     * Returns an array of nsILoginInfo.
> 
> What the heck! :)

I didn't realize that I was sneaking that in... I think I absent-mindedly fixed that - a leftover from when I wrote the original mozStorage stuff. Keep & be more correct or let that typo persist?

> >+    _getAllMatchingLogins : function (matchData) {
> ...
> >+        let hostname = formSubmitURL = httpRealm = null;
> 
> I think empty-string, not null, is the value you meant here.

I actually meant to take those out since I'm not using them...

> But...
> 
> I think it would be simpler to not use buildConditionsAndParams() here.
> Instead, have getAllLogins() pass in an empty matchData, and findLogins() pass
> in only params which are not empty-string. Then this function can just build up
> the query more or less as normal, with some minimal extra processing in the
> swtich for when these params are null (and the fun OR clause for
> formSubmitURL).

So while I wanted to do that, it's a break from the previous API used with findLogins where empty strings have to be passed in, and opted for consistency. I guess it doesn't matter so much since it's a new method.

And the whole switch can probably be simplified going in that direction. The whole null case can generalized (I just realized if you pass {id: null} it would currently throw on statement creation instead of trying to search for null id). There would still be duplication with some of the logic in _buildConditionsAndParams(), but I guess that's ok.

> 
> It would be nice if countLogins() could go through this same path (since it's
> the only other buildConditionsAndParams() caller), but I don't see an easy way
> to do that without complicating things.

Yea, too much complication there. Though if we don't need to be calling that method from searchLogins(), we can probably just integrate it into countLogins()

> >+++ b/toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_7.js
> 
> More numbers, *sadface*. I wonder if we should just call it something like
> test_mozStorage_api_searchLogins.js, and eventually move other tests towards a
> scheme like that.

Sure, there's a problem with that too - that there's a fair amount of overlap, so while it's relatively easy to do for new methods, breaking up the other tests will be interesting... Still better than numbers though
(In reply to comment #8)

> > >-     * Returns an array of nsAccountInfo.
> > >+     * Returns an array of nsILoginInfo.
> > 
> > What the heck! :)
> 
> I didn't realize that I was sneaking that in...

No, that's fine, just surprised at my (?) typo in the original. I suck. :)

> So while I wanted to do that, it's a break from the previous API used with
> findLogins where empty strings have to be passed in

The old API only did this because it was supporting a limited set of uses and as a basic API *something* had to be passed in. It's clunky. The new API doesn't need to do that -- if you don't care about the value of .foo, you just don't stick |foo| into the property bag.

> > test_mozStorage_api_searchLogins.js,
> 
> Sure, there's a problem with that too - that there's a fair amount of overlap,
> so while it's relatively easy to do for new methods, breaking up the other
> tests will be interesting... Still better than numbers though

True. Maybe "test_mozStorage_api_1.js"? (s/searchLogins/1/). That lets us still group things by rough area, but we can still be vague and just number things within an area. No big deal, just finding that it's not always clear which tests do what without peeking.
Attached patch Patch v0.3 (obsolete) — Splinter Review
Attachment #368925 - Attachment is obsolete: true
Attachment #369162 - Flags: review?(dolske)
Comment on attachment 369162 [details] [diff] [review]
Patch v0.3

>--- a/toolkit/components/passwordmgr/public/nsILoginManagerStorage.idl
...
>     /**
>+     * Fetch all logins in the login manager.

Lies. :)

>+     * @param matchData
>+     *        The data used to search. Follows the same requirements as
>+     *        findLogins for those fields. (Can be a nsIProperyBag or JS Object)

Can it really be a JS object? XPConnect magic? I'd want to verify this with someone who knows those bits better before suggesting people do that.


>--- a/toolkit/components/passwordmgr/src/storage-mozStorage.js
...
>     /*
>+     * searchLogins
>+     *
>+     * Public wrapper around _searchLogins to remove fields we don't
>+     * want to expose.

More lies! :)

>     findLogins : function (count, hostname, formSubmitURL, httpRealm) {
>         let userCanceled;
>-        let [logins, ids] =
>-            this._queryLogins(hostname, formSubmitURL, httpRealm);
>+
>+        let matchData = {
>+            hostname: hostname,
>+            formSubmitURL: formSubmitURL,
>+            httpRealm: httpRealm
>+        };
>+        let [logins, ids] = this._searchLogins(matchData);

I think it would be best to limit the "empty-string vs. null" stuff to the existing APIs, and not have searchLogin deal with it as a special case (since it doesn't need to). Our code that was using _queryLogins() would now have to do a little more work to build up matchData (basically, checking if a value was empty-string, and if so not adding it to matchData).

Probably worth noting this in the IDL, so that someone used to the quirks of findLogins() doesn't try the same thing with searchLogins().

Incidentally, I think this enables searching for logins with an empty-string formURL via searchLogins({formSubmitURL:""}), though I don't think that's particularly useful. :)
Attachment #369162 - Flags: review?(dolske) → review-
(In reply to comment #11)
> >+     * @param matchData
> >+     *        The data used to search. Follows the same requirements as
> >+     *        findLogins for those fields. (Can be a nsIProperyBag or JS Object)
> 
> Can it really be a JS object? XPConnect magic? I'd want to verify this with
> someone who knows those bits better before suggesting people do that.

Those are lies too. I was writing it so that we could do that, but it became a bit hackish & I  scrapped it. I still think it would be cool to have, though. Bonus - if we implement it later, we wont break anything that happens to use searchLogins with an propbag.

I also got to thinking that it might be worth implementing the array case (... WHERE guid IN (1,2,3)...) now as opposed to later. Since one reason we're doing this is for Weave, we might as well handle their "we have a bunch of guids to lookup" case too. I'll look into it to see if it's feasible with ease.
(In reply to comment #12)

> I also got to thinking that it might be worth implementing the array case (...
> WHERE guid IN (1,2,3)...) now as opposed to later. 

Probably not, at least not yet... Weave processes items 1 at a time, so they're not able to easily lookup a batch of guids. And I don't think we do anything like this in the browser.
Attached patch Patch v0.4 (obsolete) — Splinter Review
Fix things from comment #11. I had to take out a set of changes based on removing the empty string wildcarding. I also added one additional test case to check that the empty string lookup works for formSubmitURL
Attachment #369162 - Attachment is obsolete: true
Attachment #369756 - Flags: review?(dolske)
Comment on attachment 369756 [details] [diff] [review]
Patch v0.4

>+            switch (field) {
>+                // Historical compatibility requires this special case
>+                case "formSubmitURL":
>+                    if (field == "formSubmitURL" && value != null) {

No need to check |field| in the if().

>     findLogins : function (count, hostname, formSubmitURL, httpRealm) {
...
>+        let matchData = {
>+            hostname: hostname,
>+            formSubmitURL: formSubmitURL,
>+            httpRealm: httpRealm
>+        };
>+        for (field in matchData)
>+            if (matchData[field] == '')
>+                delete matchData[field];

I think this would be a little cleaner to do the same way getIdForLogin does it... Instead of deleting the wildcard entries from matchData, only add non-wildcard entries.


>     _getIdForLogin : function (login) {
...
>+            if (!login[field] == '')
>+                matchData[field] = login[field];

Use: if (login[field] != '')

Hrm, so, how does your version pass tests? The ! has higher precedence than ==, so it's always checking "false == ''" or "true == ''". Add another test to catch this? :)


>+matchData.setPropertyAsAString("id", "1");
>+let logins = storage.searchLogins({}, matchData);
>+do_check_eq(1, logins.length, "expecting single login with id");

One more thing... The tests should check that the returned logins are as expected, and not just the length of the results. There's code in  checkStorageData() to do this, it just needs spun out as its own function [checkLogins(array1, array2) or something].

r+ with this stuff fixed, over to mconnor to sr the interface addition.
Attachment #369756 - Flags: superreview?(mconnor)
Attachment #369756 - Flags: review?(dolske)
Attachment #369756 - Flags: review+
Attachment #369756 - Flags: approval1.9.1?
(In reply to comment #15)
> >     findLogins : function (count, hostname, formSubmitURL, httpRealm) {
> ...
> >+        let matchData = {
> >+            hostname: hostname,
> >+            formSubmitURL: formSubmitURL,
> >+            httpRealm: httpRealm
> >+        };
> >+        for (field in matchData)
> >+            if (matchData[field] == '')
> >+                delete matchData[field];
> 
> I think this would be a little cleaner to do the same way getIdForLogin does
> it... Instead of deleting the wildcard entries from matchData, only add
> non-wildcard entries.

I agree it would be cleaner, however we'll have to construct 2 objects in order to make it work the same way. I'm fine with that the way if you are, just thought this would be more efficient. I could be a little hacky and use |arguments|, but I feel like that might be worse.
(In reply to comment #15)
> Hrm, so, how does your version pass tests? The ! has higher precedence than ==,
> so it's always checking "false == ''" or "true == ''". Add another test to
> catch this? :)

That's an interesting question, especially with the loop
|for (field in ["hostname", "formSubmitURL", "httpRealm"])|
resulting in field being [0, 1, 2] and blowing up when they get into _searchLogins...

So I'm going to do
|for (i in fields = ["hostname", "formSubmitURL", "httpRealm"])|
which works, but if it's better I declare the fields outside the loop, then I will. I could also pull this whole process out into another method if we'll be adding more lines.
(In reply to comment #17)
Or maybe I'll just |for each| it... Turns out I forgot |for x in array| sets x to the index, whereas |for each x in array| sets x to the value. Lesson learned.
Attached patch Patch v0.5 (obsolete) — Splinter Review
With fixes for comment #15
Attachment #369756 - Attachment is obsolete: true
Attachment #372095 - Flags: superreview?(mconnor)
Attachment #372095 - Flags: approval1.9.1?
Attachment #369756 - Flags: superreview?(mconnor)
Attachment #369756 - Flags: approval1.9.1?
Comment on attachment 372095 [details] [diff] [review]
Patch v0.5

Please nominate for review, land, bake and then ask for approval.
Attachment #372095 - Flags: approval1.9.1?
Comment on attachment 372095 [details] [diff] [review]
Patch v0.5

Vlad: can you SR this change? Buys us better Weave perf and memory usage.
Attachment #372095 - Flags: superreview?(mconnor) → superreview?(vladimir)
Grr, I can't believe I missed this in review. The API also needs added to nsILoginManager, so it can forward the call to storage.
Comment on attachment 372095 [details] [diff] [review]
Patch v0.5

>+    void searchLogins(out unsigned long count, in nsIPropertyBag matchData,
>+                      [retval, array, size_is(count)] out nsILoginInfo logins);

Why does this put an out param before an inparam?  I understand the retval going at the end, but searchLogins(in nsIPropertyBag matchData, out unsigned long count, ...) seems much better.
(In reply to comment #23)
> Why does this put an out param before an inparam?  I understand the retval
> going at the end, but searchLogins(in nsIPropertyBag matchData, out unsigned
> long count, ...) seems much better.

I'm going to say historic reasons & consistency. All of the other methods in password manager & password manager storage work like that.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/public/nsILoginManagerStorage.idl#143
We missed the change to nsILoginManager so that consumers don't have to use the storage module directly, but can go through the Login manager API (which is preferred).

This one is for m-c (uuid revved)
Attachment #372095 - Attachment is obsolete: true
Attachment #373398 - Flags: superreview?
Attachment #372095 - Flags: superreview?(vladimir)
Attachment #373398 - Flags: superreview? → superreview?(vladimir)
Comment on attachment 373398 [details] [diff] [review]
Patch v0.6 (mozilla-central)

The only other example is countLogins(); I would suggest actually changing it as well as this if we're already revving the interface, but given that it's scriptable changing it would break existing code.  Blah, we really need to have some good idl best practices, like don't mix in and out params =/
Attachment #373398 - Flags: superreview?(vladimir) → superreview+
we didn't want to rev the uuid for branch
Pushed http://hg.mozilla.org/mozilla-central/rev/1220ac9d5526
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #373401 - Flags: approval1.9.1?
Comment on attachment 373401 [details] [diff] [review]
Patch v0.6 (mozilla-1.9.1)

a191=beltzner
Attachment #373401 - Flags: approval1.9.1? → approval1.9.1+
Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b1cae68918bd

Thunder: Weave should be good-to-go for using this API in FF3.5 Beta 4.
Keywords: fixed1.9.1
Blocks: 489268
No longer blocks: 489876
Depends on: 489876
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: