New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logins API #47

Merged
merged 13 commits into from Jan 15, 2019

Conversation

3 participants
@6a68
Copy link
Member

6a68 commented Jan 4, 2019

Fixes #4.

Testing and Review Notes

This is just the logins API as an embedded API experiment--it doesn't yet connect to the rest of the addon code, and it's marked WIP because I haven't finished the tests yet.

Most of the documentation of the Login type in the schema comes straight from the nsILoginInfo and nsILoginMetaInfo interface definitions. Because LoginManager.update() allows any of the properties to be updated, I opted to add a second LoginUpdate type with all of the properties marked optional (and adding the convenient timesUsedIncrement shortcut). The webextension API schemas don't currently specify return types, so the return info is in the description.

I've opted to structure parts of the code in keeping with in-tree API conventions, though they seem kinda odd to me; I'll call those spots out inline.


How to play around with the API:

  1. Fire up nightly via npm run run -- -f nightly
  2. Open up about:debugging
  3. Click the 'Debug' button to open a debugger
  4. In the debugger console, set up your listeners and a dummy Login by pasting in this code:
login =  {
  guid: "{33535344-9cdb-8c4a-ae10-5849d0a2f04a}",
  timeCreated: 1546291981955,
  timeLastUsed: 1546291981955,
  timePasswordChanged: 1546291981955,
  timesUsed: 1,
  hostname: "https://example.com",
  httpRealm: null,
  formSubmitURL: "https://example.com",
  usernameField: "username",
  passwordField: "password",
  username: "creativeusername",
  password: "p455w0rd"
};
browser.experiments.logins.onAdded.addListener(login => console.log("onAdded fired: ", login))
browser.experiments.logins.onUpdated.addListener(login => console.log("onUpdated fired: ", login))
browser.experiments.logins.onRemoved.addListener(login => console.log("onRemoved fired: ", login))
  1. Now you can play around with creating, updating, and removing logins:
await browser.experiments.logins.getAll();
await browser.experiments.logins.add(login);
await browser.experiments.logins.get(login.guid);
await browser.experiments.logins.getAll();
login.username = "originalusername";
await browser.experiments.logins.update(login);
await browser.experiments.logins.touch(login.guid); // note: fires onUpdated
await browser.experiments.logins.remove(login.guid);

You can try to trigger errors by adding the login twice in a row, or removing it twice in a row, or setting incorrect field types when adding or updating, etc. You can also open up the login manager (about:preferences#privacy) and make changes there, then verify that the corresponding events fire in the debugger.

Edited to change onCreated to onAdded in debugger setup script, and add touch() reference

@6a68 6a68 requested a review from mozilla-lockbox/desktop-engineering as a code owner Jan 4, 2019

@wafflebot wafflebot bot assigned 6a68 Jan 4, 2019

@wafflebot wafflebot bot added the in progress label Jan 4, 2019


const { ExtensionError } = ExtensionUtils;

const getLogins = () => {

This comment has been minimized.

@6a68

6a68 Jan 4, 2019

Member

The existing APIs seem to move functions out to API-global scope, rather than have helper functions defined on the ExtensionAPI subclass, so I went with that approach for getLogins and getLogin.

callback({ login });
},
};
Services.obs.addObserver(observer, "passwordmgr-storage-changed");

This comment has been minimized.

@6a68

6a68 Jan 4, 2019

Member

Standard practice seems to be to wire up a separate observer for each event subscriber, and do the translation from Firefox-internal to API data type inside the callback, like I've done here. Seems odd to me, but I guess we really are only going to have one listener for each event (for now), so, ehh.

Related note: the old extension set up one listener, and then used a case statement to dispatch actions. With this logins API, the addon will wire up the redux store to the LoginManager via a bunch of event listeners:

browser.experiments.logins.onCreated(login => {
  dispatcher.record({
    type: "login_changed",
    mode: "added",
    login: login,
  });
});
// same idea for onUpdated, onRemoved, onAllRemoved
"guid": {
"$ref": "Mozilla-GUID",
"optional": false,
"description": "The Mozilla-GUID to uniquely identify the login. This can be any arbitrary string, but a format as created by nsIUUIDGenerator is recommended. For example, '{d4e1a1f6-5ea0-40ee-bff5-da57982f21cf}'. (From nsILoginMetaInfo.idl)"

This comment has been minimized.

@6a68

6a68 Jan 4, 2019

Member

Note that nsILoginMetaInfo.idl actually says that this guid field "can be any arbitrary string." I have no idea if there might exist ancient logins that actually use some other identifier, instead of this uuid-in-curly-braces format. If that exists, we'll have to relax the pattern constraints on the Mozilla-GUID type.

This comment has been minimized.

@linuxwolf

linuxwolf Jan 8, 2019

Member

I would relax the constraints now. I don't think there's any old values to be worried about, but with future values.

The usefulness of GUID/UUID is starting to be questioned in the world, with the prevailing winds moving toward a hex-encoded string of a cryptographic hash process (starting with a cryptographically sufficient source string).

}, {
"id": "Login",
"type": "object",
"description": "Represents a login, with the union of properties defined in either nsILoginInfo or nsILoginMetaData.",

This comment has been minimized.

@6a68

6a68 Jan 4, 2019

Member

Whoops, just realized the idl is nsILoginMetaInfo, not nsILoginMetaData. I'll update this description.

if (!login) {
throw new ExtensionError(`Update failed: Login not found with GUID ${loginInfo.guid}`);
}
const loginAndMetaData = LoginHelper.newPropertyBag(loginInfo);

This comment has been minimized.

@6a68

6a68 Jan 4, 2019

Member

Little bit subtle, but using an nsIPropertyBag has some nice benefits and seems like the better choice for this API.

try {
Services.logins.removeLogin(LoginHelper.vanillaObjectToLogin(login));
} catch (ex) {
throw new ExtensionError(ex);

This comment has been minimized.

@6a68

6a68 Jan 4, 2019

Member

Error handling is still a little bit WIP here. If we don't rethrow errors as ExtensionErrors, then the promise is still rejected, but the original error info is destroyed, and the webextensions code emits a particularly foul "An unexpected error occurred" message...but maybe we shouldn't expose all the errors in this way. Feedback welcome :-)

This comment has been minimized.

@linuxwolf

linuxwolf Jan 8, 2019

Member

I think throwing ExtensionError is the right approach to start with.

What a consumer does with them can be worked out as we go.

This comment has been minimized.

@lmorchard

lmorchard Jan 8, 2019

Contributor

With everything else declared in the schema, I'm almost surprised we don't have to specify the inventory of exceptions that could be thrown here rather than being able to clone-by-rethrow. That's really the only thing I could think of to do differently - i.e. declare all the expected exceptions. But that feels possibly a little too boilerplatey

This comment has been minimized.

@linuxwolf

linuxwolf Jan 8, 2019

Member

It might require changes to the underlying nsILoginManager implementations to get better enumerations right now.

Eventually worth it, but for this initial work.

This comment has been minimized.

@6a68

6a68 Jan 9, 2019

Member

@lmorchard Yeah, the webextensions code in firefox doesn't validate or even parse the return values at all. I was told I could specify return types, but was afraid to do so: it's so easy to typo an unvalidated schema, and if they add return type checking, it might well break the API in the wild. I'm choosing to look at it as a "schema is half-full" situation ;-P

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Jan 8, 2019

BTW, here is the directory in m-c that has the built-in APIs that run in the parent process. The schemas are here.

@linuxwolf
Copy link
Member

linuxwolf left a comment

drive-by whilst still a WIP ...

Overall looking AWESOME! There's a question plus a few requested changed (thought we can discuss in-PR about them).

try {
Services.logins.removeLogin(LoginHelper.vanillaObjectToLogin(login));
} catch (ex) {
throw new ExtensionError(ex);

This comment has been minimized.

@linuxwolf

linuxwolf Jan 8, 2019

Member

I think throwing ExtensionError is the right approach to start with.

What a consumer does with them can be worked out as we go.

throw new ExtensionError(ex);
}
},
onCreated: new EventManager(context, "logins.onCreated", fire => {

This comment has been minimized.

@linuxwolf

linuxwolf Jan 8, 2019

Member

let's change this to "onAdded" to reconcile your concerns with naming consistency brought up elsewhere.

Show resolved Hide resolved src/experiments/logins/api.js
"guid": {
"$ref": "Mozilla-GUID",
"optional": false,
"description": "The Mozilla-GUID to uniquely identify the login. This can be any arbitrary string, but a format as created by nsIUUIDGenerator is recommended. For example, '{d4e1a1f6-5ea0-40ee-bff5-da57982f21cf}'. (From nsILoginMetaInfo.idl)"

This comment has been minimized.

@linuxwolf

linuxwolf Jan 8, 2019

Member

I would relax the constraints now. I don't think there's any old values to be worried about, but with future values.

The usefulness of GUID/UUID is starting to be questioned in the world, with the prevailing winds moving toward a hex-encoded string of a cryptographic hash process (starting with a cryptographically sufficient source string).

Show resolved Hide resolved src/experiments/logins/api.js
@lmorchard

This comment has been minimized.

Copy link
Contributor

lmorchard commented Jan 8, 2019

Modulo comments so far, this looks pretty good to me on a first read through the code and some console jiggery-pokery. I think I'll give a shot at actually hooking this up to Redux in the extension and see what I can do with it

@linuxwolf

This comment has been minimized.

Copy link
Member

linuxwolf commented Jan 8, 2019

think I'll give a shot at actually hooking this up to Redux in the extension and see what I can do with it

Currently Redux is in web content code, so this will likely need to go through the background/message-ports to keep it in the parent process.

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Jan 9, 2019

Thanks for the great feedback! Changes pushed in tasty little commits. I'll re-request review and remove the WIP in the title when I've got the tests fully working and pushed to this branch.

@6a68 6a68 force-pushed the 6a68:logins-api branch from df4cff3 to 86a4417 Jan 9, 2019

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Jan 9, 2019

Rebased & force-pushed

@linuxwolf

This comment has been minimized.

Copy link
Member

linuxwolf commented Jan 9, 2019

I'll re-request review and remove the WIP in the title when I've got the tests fully working and pushed to this branch.

Works for me! Also dismiss my previous review when you're ready. I'll be sure to resolve the things that are done via discuss.

WIP: so close, got nothin
When webdriver opens the extension page, the browser.experiments.logins
API is not present. When I open the extension page myself, via going to
about:debugging, and doing
  browser.tabs.create({
    url: browser.extension.getURL(
           "/test/integration/test-pages/logins-api.html")
  }),
the test page seems just fine. Maybe webdriver needs some magic powers?
@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Jan 10, 2019

@lmorchard OK, since you're poking at selenium things, here's tonight's puzzler: I can open the test page I've created, by doing the following:

  • NODE_ENV=test npm run run -- -f nightly at the command line
  • open up about:debugging
  • in the console, do browser.tabs.create({url: browser.extension.getURL("/test/integration/test-pages/logins-api.html")})

And then, in the resulting test page, I can click the buttons and see stuff happen, and/or check for the existence of browser.experiments.logins in the console. So far, so good.

Now, if I run the new integration test†, there's a 30 second wait after that same test page loads, in which I can manually check that the buttons don't work, and looking in the console, I don't see anything under browser.experiments. What's up with that? No idea.

† There might be a cleaner incantation, but I've been running just this specific integration test via:
./node_modules/.bin/cross-env NODE_ENV=test ./node_modules/.bin/mocha --require @babel/register --timeout 10000 test/integration/logins-api-test.js

@lmorchard

This comment has been minimized.

Copy link
Contributor

lmorchard commented Jan 10, 2019

OK, since you're poking at selenium things

Well, I'm not actually doing that yet, and don't really have any advice there :/ Will probably dig into that after more work on #21 and getting this API working in the addon

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Jan 10, 2019

I'm going to try adding a test-only browser toolbar button. Maybe that'll open the page with the right permissions

@lmorchard

This comment has been minimized.

Copy link
Contributor

lmorchard commented Jan 10, 2019

@6a68 Okay, so this is super underwhelming and I need to update tests, but I hooked up the Logins API to the datastore and it does seem to work in both directions for managing logins (i.e. from the add-on and from built-in options):

https://github.com/6a68/lockbox-addon/compare/logins-api...lmorchard:21-logins-api-hookup?expand=1

Sharing this WIP against your WIP in case it helps with anything.

I should have had this working a day or two ago, but I kind of went down a premature rabbit hole chasing issue #21.

I started playing with an overhaul replacing datastore with Redux store code shared between background script and frontend UI. Spent a little too long on that. Might still be a good idea, but going to shelve it for now and focus on getting these more minimal changes at least wrapped in unit tests.

6a68 added some commits Jan 10, 2019

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Jan 10, 2019

Turns out the problem was that we need to set the legacy.extensions.enabled pref to true, otherwise the mozillaAddons pref is stripped out of the manifest, and the API experiment isn't loaded. 🤦‍♂️ 🤦‍♂️ 🤦‍♂️

I've added the happy path tests. We can go super-deluxe and add the error case tests here too, or defer the error cases to a future bug.

Time for another round

@6a68 6a68 requested review from linuxwolf and lmorchard Jan 10, 2019

@6a68 6a68 changed the title WIP logins API Logins API Jan 10, 2019

await clickButton("update");

const logins = await getLogins();
expect(logins[0].username).to.equal(mockLogin.username);

This comment has been minimized.

@6a68

6a68 Jan 10, 2019

Member

Oh, this is wrong. The username shouldn't match the initial value. I'll fix.

This comment has been minimized.

@6a68

6a68 Jan 11, 2019

Member

I've got something wrong with the update() method. Clearing review requests for now.

@6a68 6a68 removed request for lmorchard and linuxwolf Jan 11, 2019

Final tweaks to update() API method
* The webextensions framework auto-inserts 'null' default values for any
specified keys that are missing, causing updates to break
LoginManager.modifyLogin() type checking. Filter out those null values.

* Create a coding convention to make clearer which things are vanilla JS
objects with login fields, which are nsILoginInfo objects, and which are
nsIPropertyBags used to modifyLogin.

@6a68 6a68 requested review from linuxwolf and lmorchard Jan 11, 2019

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Jan 11, 2019

Updated. I'm not sure why, but running npm run integration, I get a SessionNotCreatedError when starting the logins test. Maybe something in the functional test isn't getting cleaned up? Not sure, looking into it. For now, you can verify that logins test alone works by using the incantation

./node_modules/.bin/cross-env NODE_ENV=test ./node_modules/.bin/mocha --require @babel/register --timeout 10000 test/integration/logins-api-test.js

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Jan 11, 2019

Think I've got it. Should be green with this run

}
// The addons framework code always supplies null values for missing keys,
// so filter those out.
// TODO: Could this cause subtle bugs? Would anyone ever want to update a field to be null? If so, maybe default to 'NULL" (the string), or undefined.

This comment has been minimized.

@6a68

6a68 Jan 11, 2019

Member

This is my one remaining point of uncertainty. Feedback welcome

This comment has been minimized.

@linuxwolf

linuxwolf Jan 15, 2019

Member

I did not realize this is the case. This null behavior seems counter to "common idioms" with JSON-ish usage pattern (using null to clear a value, and undefined or omitted to ignore), arguably best embodied in JSON merge patch.

I would lean toward newLogin then having to be the complete representation rather than the subset that should actually be updated.

This comment has been minimized.

@linuxwolf

linuxwolf Jan 15, 2019

Member

To be clear, I don't think this behavior will have any impact on us in practice --- for now. If we ever try to tackle toggling between "HTTP auth" and "submit form", this will cause problems.

@6a68 6a68 requested review from mozilla-lockbox/desktop-engineering and removed request for linuxwolf and lmorchard Jan 11, 2019

@linuxwolf
Copy link
Member

linuxwolf left a comment

r+

It look great to me. I think it's good to land now regardless the direction the feedback portion goes. I think it's fine for that to be follow-up issue(s) and changes -- even if they are schema breaking (prior to landing in-tree).

}
// The addons framework code always supplies null values for missing keys,
// so filter those out.
// TODO: Could this cause subtle bugs? Would anyone ever want to update a field to be null? If so, maybe default to 'NULL" (the string), or undefined.

This comment has been minimized.

@linuxwolf

linuxwolf Jan 15, 2019

Member

I did not realize this is the case. This null behavior seems counter to "common idioms" with JSON-ish usage pattern (using null to clear a value, and undefined or omitted to ignore), arguably best embodied in JSON merge patch.

I would lean toward newLogin then having to be the complete representation rather than the subset that should actually be updated.

}
// The addons framework code always supplies null values for missing keys,
// so filter those out.
// TODO: Could this cause subtle bugs? Would anyone ever want to update a field to be null? If so, maybe default to 'NULL" (the string), or undefined.

This comment has been minimized.

@linuxwolf

linuxwolf Jan 15, 2019

Member

To be clear, I don't think this behavior will have any impact on us in practice --- for now. If we ever try to tackle toggling between "HTTP auth" and "submit form", this will cause problems.

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Jan 15, 2019

Thanks @linuxwolf! Filed #58 to address your final round of feedback. Squashing and merging.

@lmorchard lmorchard merged commit 698295f into mozilla-lockbox:master Jan 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment