1324919 - Land logins API

Status: NEW

(WebExtensions :: Experiments, defect, P5)

Description

[Andy McKay]

We landed logins as an experiment in bug 1285270. This bug is to move it into Firefox. To do this we'll need to address the bugs on the API and ask for a security review of the API.

https://github.com/web-ext-experiments/logins
https://github.com/web-ext-experiments/logins/issues

Comment 1

[Matthew N. [:MattN]]

I just filed a few issues on GitHub that I think we should address to reduce confusion for consumers: https://github.com/web-ext-experiments/logins/issues?utf8=%E2%9C%93&q=is%3Aissue%20author%3Amnoorenberghe%20

Comment 2

[Jim Porter (:squib)]

On the password manager team, we're looking into adding a WebExtension API to let extensions register themselves as the *source* of login information. This would let us (or other password managers) integrate more-smoothly with Firefox by letting us take advantage of the existing form autocomplete code. This is still very, *very* early (I'm currently just experimenting with what the API would look like), and I don't think we've decided for sure if this is the route we want to take, but I thought I'd give the folks in this bug a heads-up that we'll likely be looking at adding this sort of functionality to the `browser.logins` API.

I'm not sure what (if any) coordination we need here, but I'd be happy to discuss where we'd like to go in the medium term with password management in Firefox.

Comment 3

[Andrew Swan [:aswan]]

(In reply to Jim Porter (:squib) from comment #2)
> On the password manager team, we're looking into adding a WebExtension API
> to let extensions register themselves as the *source* of login information.
> This would let us (or other password managers) integrate more-smoothly with
> Firefox by letting us take advantage of the existing form autocomplete code.
> This is still very, *very* early (I'm currently just experimenting with what
> the API would look like), and I don't think we've decided for sure if this
> is the route we want to take, but I thought I'd give the folks in this bug a
> heads-up that we'll likely be looking at adding this sort of functionality
> to the `browser.logins` API.
> 
> I'm not sure what (if any) coordination we need here, but I'd be happy to
> discuss where we'd like to go in the medium term with password management in
> Firefox.

I think that's an excellent idea, I think bug 1344788 is meant to cover it, perhaps that would be a good place to continue discussion.

Comment 4

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1324919 - Land logins API,

Review commit: https://reviewboard.mozilla.org/r/130860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130860/

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8858881 [details]
Bug 1324919 - Land logins API,

https://reviewboard.mozilla.org/r/130860/#review133872

::: toolkit/components/extensions/ext-logins.js:40
(Diff revision 1)
> +    dump(`new uri failed ${ex.message}\n`);
> +    // unparseable hostname, can this actually happen?

let's turn this into `Cu.reportError()`

::: toolkit/components/extensions/ext-logins.js:55
(Diff revision 1)
> +            || url.host == context.extension.uuid);
> +  }
> +  return (context.extension.whiteListedHosts.matches(url));
> +}
> +
> +function checkFields(context, info) {

Can we give this a more descriptive name?  And add a comment documenting it?
Also the semantics seem goofy to me (the presence of a message property in the returned object indicating an error -- why not just throw or return an Error object)

::: toolkit/components/extensions/ext-logins.js:101
(Diff revision 1)
> +          let logins = Services.logins.findLogins({}, query.origin || "",
> +                                                  query.formActionOrigin || "",
> +                                                  query.httpRealm || "")

please make "" the default in the schema and skip the || here

::: toolkit/components/extensions/ext-logins.js:128
(Diff revision 1)
> +            return Promise.reject({message: err.message});
> +          }
> +          return Promise.resolve();
> +        },
> +
> +        update(info) {

This seems more natural to me with `id` as a separate parameter rather than plucked from `info`.

::: toolkit/components/extensions/ext-logins.js:131
(Diff revision 1)
> +        },
> +
> +        update(info) {
> +          if (!info.id) {
> +            return Promise.reject(
> +              {message: "LoginItem passed to modify must have an id."});

modify -> update

::: toolkit/components/extensions/ext-logins.js:146
(Diff revision 1)
> +          let newLoginData = new LoginInfo(origin, info.formActionOrigin,
> +                                           info.httpRealm, info.username,
> +                                           info.password, "", "");

In the schema, all these properties are optional, shouldn't they be copied from the original entry if they aren't specified?

::: toolkit/components/extensions/schemas/logins.json:14
(Diff revision 1)
> +      {
> +        "$extend": "OptionalPermission",
> +        "choices": [{
> +          "type": "string",
> +          "enum": [
> +            "logins"

This seems like its also worthy of a permisison string.

::: toolkit/components/extensions/test/xpcshell/test_ext_logins.js:11
(Diff revision 1)
> +  for (let prop of Object.keys(expected)) {
> +    equal(actual[prop], expected[prop], `Expected value for ${prop} for found login.`);
> +  }
> +}
> +
> +add_task(function* test_logins() {

use async instead of a generator

::: toolkit/components/extensions/test/xpcshell/test_ext_logins.js:73
(Diff revision 1)
> +  let newLogin = Services.logins.findLogins({}, login1.origin, "", "")[0];
> +  ok(newLogin, "Login created and found via nsILoginManager.");
> +  newLogin.QueryInterface(Ci.nsILoginMetaInfo);

do you need to look this up?  can you just QI on `info`?

::: toolkit/components/extensions/test/xpcshell/test_ext_logins.js:98
(Diff revision 1)
> +
> +  // But with non-matching search terms we should not see it.
> +  response = yield run(privilegedExtension, "search", {username: "somebodyelse"});
> +  equal(response.results.length, 0, "No logins found.");
> +
> +  // Test each field and some combinations.

These tests would be more meaningful if there was more than one stored record.

::: toolkit/components/extensions/test/xpcshell/test_ext_logins.js:100
(Diff revision 1)
> +    {formActionOrigin: login1.formSubmitURL},
> +    {origin: login1.hostname},

login1 has the extension-visible property names, not the nsILoginInfo property names...

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8858881 [details]
Bug 1324919 - Land logins API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130860/diff/1-2/

Comment 7

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8858881 [details]
Bug 1324919 - Land logins API,

https://reviewboard.mozilla.org/r/130860/#review133872

> do you need to look this up?  can you just QI on `info`?

Yes, it needs to be looked up. After calling `Services.logins.addLogin(info)` the id is not updated in the info object. If I call `info.QueryInterface(Ci.nsILoginMetaInfo)` and then check the value of `info.guid` it is `null`, so I think I have to retrieve the login object in order to find the `guid`.

It also seems like a decent check to make sure that the login was actually added successfully.

> These tests would be more meaningful if there was more than one stored record.

I've stored an extra record, and also added a few more tests for other things.

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8858881 [details]
Bug 1324919 - Land logins API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130860/diff/2-3/

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8858881 [details]
Bug 1324919 - Land logins API,

https://reviewboard.mozilla.org/r/130860/#review135888

as discussed this morning, clearing review until after the security review.

Comment 10

[Matthew Miller [:m_and_m]]

On the password manager team, we're still evaluating what the schema going forward is.  I'm concerned about getting too locked into the schemed documented here.  Specifically:

* "origin" -- there is strong desire to support more than "protocol://hostname[:port]" (e.g., path and/or URI template); consider relaxing the "should omit anything after the hostname and (optional) port"
* "formActionOrigin" -- we're not convinced the future schema will still support this; consider suppressing this field

Comment 11

[Bob Silverberg [:bsilverberg]]

(In reply to Matthew Miller [:m_and_m] from comment #10)
> On the password manager team, we're still evaluating what the schema going
> forward is.  I'm concerned about getting too locked into the schemed
> documented here.  Specifically:
> 
> * "origin" -- there is strong desire to support more than
> "protocol://hostname[:port]" (e.g., path and/or URI template); consider
> relaxing the "should omit anything after the hostname and (optional) port"
> * "formActionOrigin" -- we're not convinced the future schema will still
> support this; consider suppressing this field

Thanks Matthew, I appreciate your taking a look and offering some guidance. I will make those changes in the next iteration of the patch.

Comment 12

[Bob Silverberg [:bsilverberg]]

Attachment: Master password prompt

One issue that exists with this API is that if a user has a master password enabled, they will see a prompt for the master password the first time any of the API methods are called. This might be confusing to a user as there is no context for this prompt, and the prompt itself is kind of confusing (see attached), although I suppose anyone with a master password enabled would be familiar with the prompt.

I think at the very least we need to document this, so extension developers will know what their users can expect. We may even want to think about bug 1359182 in relation to this, which is requesting an API for interacting with the master password. That bug is currently sitting in untriaged, however, so there may be reasons we don't want to address it. For example, I'm not even sure what the future holds for master passwords in general.

Comment 13

[Matthew Miller [:m_and_m]]

(In reply to Bob Silverberg [:bsilverberg] from comment #12)
> Created attachment 8861939 [details]
> Master password prompt
> 
> One issue that exists with this API is that if a user has a master password
> enabled, they will see a prompt for the master password the first time any
> of the API methods are called. This might be confusing to a user as there is
> no context for this prompt, and the prompt itself is kind of confusing (see
> attached), although I suppose anyone with a master password enabled would be
> familiar with the prompt.
> 
> I think at the very least we need to document this, so extension developers
> will know what their users can expect. We may even want to think about bug
> 1359182 in relation to this, which is requesting an API for interacting with
> the master password. That bug is currently sitting in untriaged, however, so
> there may be reasons we don't want to address it. For example, I'm not even
> sure what the future holds for master passwords in general.

Personally, I think documenting the warning is the best course of action for now.

Comment 14

[Tamas Hubai [:htamas]]

I also think people using a master password expect seeing the prompt. Documenting it won't hurt though, nor adding some context to the prompt text (see bug 657871).

As for bug 1359182 about the master password API, I just filed it two days ago, that's why it's sitting untriaged. I do hope it will get some attention.

Comment 16

[Sierk Bornemann]

[Tracking Requested - why for this release]:

Comment 17

[Michael Vogt]

Sorry for nagging on here. I'm in the process of porting a legacy extension ("passhash") into the (not so) new world of webextensions. One missing piece for me is this new logins API. Any estimates if this might be ready for ff58?

Comment 18

[Bob Silverberg [:bsilverberg]]

This will definitely not be done for 58.

Comment 20

[development]

Sorry for nagging as well. 

All I can see is that this API is not going to get into FF58. But there is no timeline and no alternative. It would be great to get one. The proposed API would work fine for us (see proposal from Bob Silverberg [:bsilverberg] on Bug 1357856 - https://github.com/web-ext-experiments/logins/blob/master/api.md).

For us this is the only way to run FF57+ because we need to transfer logins to and from different browsers on different operating systems, so Firefox Sync will not work (anyway it is not usable due to our security policy). It gets all the more urgent because the long term stable release gives us updates only up to March 2018. 

Kind Regards
Franz Gotsis
Munich, Germany

Comment 21

[Future]

Me, as a power user of Mozilla Firefox, use my self-hosted Firefox Sync server to share passwords across many devices. Besides all the nice features of Firefox Sync, it was very convenient to store passwords in the password manager of Firefox browser, provided you can make use of several password related addons (e.g. Saved Password Editor, QuickPasswords, MasterPassword+). I prefer Firefox over other cloud providers, because I am in control of my data - this is why we love Firefox!  With Quantum, they all do not work anymore and the builtin password manager is lacking a lot of these functionalities.

Please consider implementing an API that developers can migrate their password related addons to make us power users happy again :-)

Comment 22

[Anselmo Canfora]

(In reply to Future from comment #21)
> Me, as a power user of Mozilla Firefox, use my self-hosted Firefox Sync
> server to share passwords across many devices. Besides all the nice features
> of Firefox Sync, it was very convenient to store passwords in the password
> manager of Firefox browser, provided you can make use of several password
> related addons (e.g. Saved Password Editor, QuickPasswords,
> MasterPassword+). I prefer Firefox over other cloud providers, because I am
> in control of my data - this is why we love Firefox!  With Quantum, they all
> do not work anymore and the builtin password manager is lacking a lot of
> these functionalities.
> 
> Please consider implementing an API that developers can migrate their
> password related addons to make us power users happy again :-)

As another FF user I definitely agree with you and join your request to FF developers.

Comment 24

[Shane Caraveo (:mixedpuppy)]

We should decide whether this is happening, then maybe start over with fresh bugs.

Comment 26

[Ben Bucksch (:BenB)]

+1 for happening

I need access to passwords storage for almost all my addons, but usually only to store passwords in a secure way and then read them back. I just want to store my own passwords that the user gave me in a secure way.

Bug 1285270 asks to read the password for one specific site, for addons that augment and use that site and to avoid double login. That's also fair, if that's clearly shown to the user in the permissions.

Accessing all existing passwords stored by other parties (browser, other addons) for any site is a whole different ball game and should be a separate permission than storing and reading my own passwords.

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

From my perspective this is happening, but I'd like to get in touch with a few stakeholders before we proceed. I feel there are different use cases to a logins API that we've tried to cover in one go:

1. Provide a secure way for add-ons to store sensitive information, such as login tokens and passwords. This storage is add-on specific.
2. Provide a way for add-ons to access passwords for websites and autofill.

My current thinking involves implementing (1) in a way that the sensitive data is only available to the add-on itself. I'm not set on which storage to use for this, but ideally it should be behind the same security mechanism Firefox uses (e.g. master passwords, or whatever the evolution of that will be).

For (2) I feel that the risk is too great to offer read access to this information. While there are certainly other ways to steal sensitive information that don't involve a passwords/logins permission, having an API that provides direct read access for developers without any hoops seems careless. What I could imagine is a write-only API that would allow add-ons to save information for websites, without obtaining access to it.

Comment 28

[Daniel Dawson]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #27)

2. Provide a way for add-ons to access passwords for websites and autofill.

I would definitely be interested in this case, as one who previously had two add-ons doing this on Firefox, up until legacy add-ons were cut off.

For (2) I feel that the risk is too great to offer read access to this information. While there are certainly other ways to steal sensitive information that don't involve a passwords/logins permission, having an API that provides direct read access for developers without any hoops seems careless.

Does having to ask permission and let the user see that it can do this not count as some sort of hoop? Is there no way to mitigate the risk? FWIW, I would be in favor of having separate read and write permissions, so add-ons that only need write access can ask only for the less risky permission.

Comment 29

[Russ Hansen]

If the permission was to be asked, there would have to be safeguards built in, or Firefox would have to inform the user that giving permission is extremely risky and put in an "I know what I'm doing" checkbox. It seems rather inadequate as a hoop. An add-on would also need to be isolated from other add-ons so the add-on with the password permission can't pass the passwords off to some other (possibly nefarious) extension. So each extension should have it's own isolated access to the password list in the API.

One thought is that logins API could only be used by an extension if the user has set a master password (MP) on the login storage, and make it use the MP every time it wants to access it, even if the MP has already been entered in a given session. This would be like the additional MP entry needed to view passwords in the current UI. A dialogue to the effect of "[extension] wants to access your password list. Enter the master password to give permission. Press cancel and uninstall the extension if you do not expect this behavior."

Comment 30

[blk]

To follow up on Philipp Kewish's comment (comment #27), there's a third use case that is also relevant for the logins API: allowing the user to store passwords in a secure location outside of Firefox - specifically, system password stores such as the Windows Credential Manager, macOS's Keychain, or the GNOME Keyring in various flavors of Linux.

I myself, before the old nsiLoginStorage API was deprecated, was using an extension that stored passwords in the macOS Keychain instead of Firefox's storage.

Comment 31

[stefan.mueller.83]

If you integrate the features of the good old Saved Password Editor
https://addons.thunderbird.net/de/thunderbird/addon/saved-password-editor/?src=userprofile
https://legacycollector.org/firefox-addons/60265/index.html
you may can reduce the requirements on this API

Comment 32

[Daniel Dawson]

That would eliminate any need for that one add-on, sure. But not any others. My other relevant add-on allowed users to manage metadata (things like tags and notes) for logins; it used hashed usernames as lookup keys. There was also Password Exporter, which did exactly what the name implies, as well as importing them. Those also required full read access.

Comment 33

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 4 duplicates, 26 votes and 62 CCs.
:robwu, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 34

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Comment 35

[Octavio Rodríguez]

Ping status. Is there "Login Manager API" in Firefox?. Is there an API that allows access to saved logins in Firefox?