Make store() modify existing entries

[aswan]

This is documented in api.md but not yet implemented, should be a straightforward change.

[jonathanKingston]

@aswan the api isn't clear but would it store allow for multiple accounts with different usernames?

The API suggests: If there is an existing record with the same hostname, formSubmitURL, and realm how would I go about storing two logins?

[aswan]

Good question. Just making username part of the key would address the immediate issue here (having credentials for multiple accounts on the same site) but then you couldn't use modify to update an existing username.

Since there isn't a universal concept of which fields are keys and which are not, I think probably the best thing here is the simplest: provide an API method that looks more or less like nsILoginManager.modifyLogin() (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsILoginManager#modifyLogin())

@mixedpuppy any comments?

[mixedpuppy]

Yeah, username would be a necessary part to handle multiple accounts. I might need to do a dump of my password database and see what I have in there. I'm not sure if other fields may also differentiate. e.g. same hostname/username but different formurl or realm, but I'm sure that would be less common. Now that I'm thinking about this more...

Thinking about how I would use the api...

logininfo = login.search({ hostname: "sillywalks", username: "cleese" }) 
if logininfo then
  // we would want an exact match here, not a query that could 
  // remove more than I want to
  login.remove(logininfo)
end
// make my modifications
logininfo.password = "fubar"
login.store(newlogininfo)

vs.

logininfo = login.search({ hostname: "sillywalks", username: "cleese"  }) 
logininfo.password = "foobar"
login.store(logininfo)

Which requires the api to do all the heavy lifting, but is much less likely to have bugs from the extension side. It would also require a second search inside store to fetch the original nsILoginInfo since I've modified it (unless we keep the originals somehow). If I wanted to change a domain, say journal.me.com to blog.me.com, it becomes much more difficult to keep this simple. remove/modify require matching original fields. This is where a separate modify api might be better.

logininfo = login.search({ hostname: "sillywalks" }) 
login.modify(logininfo, { hostname: "ministry" })

[bobsilverberg]

I'm taking on landing this API in Firefox and want to address all outstanding issues and I'm wondering where we stand on this.

Do we want to add a modify method to the API for cases in which the extension knows it wants to modify a record? There will be plenty of cases in which an extension doesn't know if a new record is being added, or an existing one is being modified, so I think keeping store and allowing it to modify an existing record, if one exists, is a good idea, and for that I think we do need to add username to the key.

[bobsilverberg]

@aswan @mixedpuppy I'm looking for feedback on my comment above, when you have a moment.

[aswan]

I think I made a mistake in making this look like the sdk api. It works fine for search(), store(), and remove() but totally breaks for modify. It looks like the underlying records implement nsILoginMetaInfo as well as nsILoginInfo and the MetaInfo interface contains a guid. Off the top of my head, including that guid in the objects returned by search() and then taking that as a parameter to a new modify() method (and changing remove() to use it as well) sounds pretty clean. I would definitely run that by @mnoorenberghe though, that could actually not work for many reasons (for instance can you search by guid in nsILoginManager.searchLogins()? i'm not sure...)

[mnoorenberghe]

I think I made a mistake in making this look like the sdk api. It works fine for search(), store(), and remove() but totally breaks for modify. It looks like the underlying records implement nsILoginMetaInfo as well as nsILoginInfo and the MetaInfo interface contains a guid. Off the top of my head, including that guid in the objects returned by search() and then taking that as a parameter to a new modify() method (and changing remove() to use it as well) sounds pretty clean.

I'm in favor of using the guid as a strong indicator that a modification should happen instead of an add (please don't expose nsILoginInfo.id) but note that pwmgr does assume (and tries to enforce, though not perfectly) the following primary key:

(hostname, httpRealm, formSubmitURL, username)

so if you get an "add" where all 4 of the above match an existing record you need to either throw an error or treat it as a modification to the matching record or else you will cause all sorts of problems for password manager.

I would definitely run that by @mnoorenberghe though, that could actually not work for many reasons (for instance can you search by guid in nsILoginManager.searchLogins()? i'm not sure...)

Yes, you can, and that's what sync does and another place in pwmgr IIRC so it should work fine.

[bobsilverberg]

From this discussion it sounds like we can keep the API as it is, with just providing a store method, which keeps things simple for consumers. We would then implement store so that it checks for an id (which would come from nsILoginInfo.guid) and if one is found we use nsILoginManager.modifyLogin(). If one is not found we can use nsILoginManager.addLogin(), as we are doing now, although it sounds like we may need an extra check to make sure we aren't trying to add a login which results in a duplicate record for (hostname, httpRealm, formSubmitURL, username).

One issue with that, though, is that @mnoorenberghe said "please don't expose nsILoginInfo.id", but we would be exposing nsILoginInfo.guid as the id, which I am guessing is the same thing. Is it important that that specific id not be made available to any consumers?

@mnoorenberghe @aswan Other than my last question, does the above all make sense, or am I missing something here?

[aswan]

I would vote for adding a separate modify() method to completely remove the question of when-is-a-store-a-modify. That's an additional method but I think it makes everything more clear for users of the API, not less.
As to whether you call the property that callers see id or guid, id would be more consistent with other existing webextension apis but I don't feel strongly about it.

[andymckay]

Minor nit, let's try and keep our APIs consistent with create, update, remove, get etc. (history and bookmarks use those names for example).

[mnoorenberghe]

One issue with that, though, is that @mnoorenberghe said "please don't expose nsILoginInfo.id", but we would be exposing nsILoginInfo.guid as the id, which I am guessing is the same thing. Is it important that that specific id not be made available to any consumers?

I'm fine with you calling the guid id in your API but internally in the login storage backends there is an auto-incrementing integer id field that shouldn't be exposed as it's a legacy thing that unfortunately was ported to the JSON backend from the mozStorage one.

@mnoorenberghe @aswan Other than my last question, does the above all make sense, or am I missing something here?

Well, I care about how you handle the following which you didn't make a decision on:

although it sounds like we may need an extra check to make sure we aren't trying to add a login which results in a duplicate record for (hostname, httpRealm, formSubmitURL, username).

I also think separate APIs for adding and modifying is more clear and I also think you should align method names with your other APIs. You will still have to handle that your create/add method may fail if a duplicate exists so that should be documented in the API docs and a test should be added for that case.

[bobsilverberg]

Well, I care about how you handle the following which you didn't make a decision on:

although it sounds like we may need an extra check to make sure we aren't trying to add a login which results in a duplicate record for (hostname, httpRealm, formSubmitURL, username).

I'm not sure how we could create a problem here. I wrote a test that calls nsILoginManager.addLogin() with a duplicate login, and nsILoginManager threw its own error: "This login already exists". I think we're fine with that error being thrown in this case, so do we really need to check for a duplicate in our code before calling nsILoginManager.addLogin()?

[mnoorenberghe]

Well it would be a problem for consumers if you didn't propagate the exception e.g. by catching it. If the plan is to propagate the error (or propagate a custom Error) then that's fine with me. I confirmed that addLogin is doing checks for duplicates[1] (along with some other checks you may want to document) and also ensuring that the GUID is unique add creation time.

[1] https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/toolkit/components/passwordmgr/nsLoginManager.js#272-313

[bobsilverberg]

Thanks @mnoorenberghe. We do catch and propagate the error, so I think we're okay.

[bobsilverberg]

This has been addressed in the patch for [1]. I will mark this as fixed when that patch lands.

[1] https://bugzil.la/1324919