Status

defect
P5
normal
2 years ago
13 days ago

People

(Reporter: andy+bugzilla, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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

Updated

2 years ago
Component: WebExtensions: General → WebExtensions: Experiments
Whiteboard: triaged
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED

Updated

2 years ago
Depends on: 1285270
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
(Reporter)

Updated

2 years ago
webextensions: ? → ---
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.
(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 hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Blocks: 1357441

Comment 5

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Depends on: 1357856

Comment 9

2 years ago
mozreview-review
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.
Attachment #8858881 - Flags: review?(aswan)
See Also: → 1344788
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
(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.
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.
(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.
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.

Updated

2 years ago
See Also: → 1390557
Duplicate of this bug: 1393683

Comment 16

2 years ago
[Tracking Requested - why for this release]:

Comment 17

2 years ago
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?
This will definitely not be done for 58.
Assignee: bob.silverberg → nobody
Status: ASSIGNED → NEW
Priority: P3 → P5

Updated

2 years ago
Duplicate of this bug: 1422323
Comment hidden (advocacy)
Comment hidden (advocacy)
Comment hidden (advocacy)

Updated

11 months ago
Product: Toolkit → WebExtensions
Duplicate of this bug: 1500842
You need to log in before you can comment on or make changes to this bug.