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

Disable prompts to save logins while on extension pages #74

Merged
merged 1 commit into from Feb 11, 2019

Conversation

4 participants
@lmorchard
Copy link
Contributor

lmorchard commented Feb 6, 2019

  • Add getLoginSavingEnabled and setLoginSavingEnabled methods to the
    Logins experiment API

  • Disable login saving for extension pages on background startup

Fixes #57

@lmorchard lmorchard requested a review from mozilla-lockbox/desktop-engineering as a code owner Feb 6, 2019

@wafflebot wafflebot bot added the in progress label Feb 6, 2019

await disableRememberSignons();
});
it("does not enable login saving for origin when given true", async () => {
await commonTest(true, false);

This comment has been minimized.

@lmorchard

lmorchard Feb 6, 2019

Author Contributor

Mild corner case here: If the user has turned off saving logins globally in options, getLoginSavingEnabled always returns false no matter what the setting might be.

This comment has been minimized.

@6a68

6a68 Feb 8, 2019

Member

Good catch! I'm not sure which, if any, parts of the logins API will work if logins are globally disabled. I think this needs investigation; could you file a bug?

This comment has been minimized.

@linuxwolf

linuxwolf Feb 8, 2019

Member

From my own experiences, the CRUD portion of the upstream API works just fine regardless of what the global pref and this specific callout are set to.

const origin = browser.extension.getURL("").slice(0, -1);
const savingEnabled = await browser.experiments.logins
.getLoginSavingEnabled(origin);
if (savingEnabled) {

This comment has been minimized.

@lmorchard

lmorchard Feb 6, 2019

Author Contributor

Checking whether saving is enabled to ensure we keep it off. The user could enable login save globally or manually delete the exception from the list in options. Kind of edge casey, but bleh.

This comment has been minimized.

@6a68

6a68 Feb 8, 2019

Member

It seems unlikely that users are going to mess with this manually. That said, the browser APIs are available from extension pages, so we could just run this check when the management page is loaded (or, to get kind of extreme about the "user messes things up" edge case, we could run this check on page load and whenever the page visibility API's visibilitychange event fires).

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

lmorchard commented Feb 6, 2019

Hmm, so I only ensure saving is off on startup of the addon. The user could later enable saving in options or delete the exception - and then we'd see the login save prompt come up until next add-on startup.

I wonder if I should make a check more often? Like, say, whenever the browser action button is clicked?

@meandavejustice
Copy link
Contributor

meandavejustice left a comment

This is working well for me.

Hmm, so I only ensure saving is off on startup of the addon. The user could later enable saving in options or delete the exception - and then we'd see the login save prompt come up until next add-on startup.
I wonder if I should make a check more often? Like, say, whenever the browser action button is clicked?

I think it would be pretty rare for someone to go and manually delete that rule, If they did I think it's fine to respect it until the next addon startup.

@linuxwolf

This comment has been minimized.

Copy link
Member

linuxwolf commented Feb 8, 2019

I wonder if I should make a check more often? Like, say, whenever the browser action button is clicked?

I think we can start with this, and see how it goes. If we do it more frequently, I'd argue for whenever we think the management page is loaded.

@linuxwolf
Copy link
Member

linuxwolf left a comment

LGTM :shipit:

@6a68
Copy link
Member

6a68 left a comment

Great work overall! Technically, I think this can land as-is, but I do have some suggestions to consider.

@@ -49,6 +49,20 @@ this.logins = class extends ExtensionAPI {
// - fooLogin = login as vanilla JS object
// - fooLoginInfo = login as nsILoginInfo
// - fooBag = (subset of) login fields as nsIPropertyBag
getLoginSavingEnabled(aHost) {

This comment has been minimized.

@6a68

6a68 Feb 8, 2019

Member

I really dislike the aFoo naming convention. How about just calling this origin?

This comment has been minimized.

@lmorchard

lmorchard Feb 8, 2019

Author Contributor

Heh, I used aHost to match the upstream API, but I can change it if necessary

@@ -49,6 +49,20 @@ this.logins = class extends ExtensionAPI {
// - fooLogin = login as vanilla JS object
// - fooLoginInfo = login as nsILoginInfo
// - fooBag = (subset of) login fields as nsIPropertyBag
getLoginSavingEnabled(aHost) {
try {
return Services.logins.getLoginSavingEnabled(aHost);

This comment has been minimized.

@6a68

6a68 Feb 8, 2019

Member

The trailing slash thing seems like a super annoying gotcha. I think we should either handle it here (strip it out or throw if it's detected), or document this annoyance in the schema docs.

{
"name": "getLoginSavingEnabled",
"type": "function",
"description": "For the given origin, gets a boolean flag which determines whether login saving is enabled",

This comment has been minimized.

@6a68

6a68 Feb 8, 2019

Member

Should also mention that the returned promise is rejected if the URL is malformed.

"parameters": [{
"name": "aHost",
"type": "string",
"description": "The origin for which to check the status of login saving"

This comment has been minimized.

@6a68

6a68 Feb 8, 2019

Member

Just use the description from the IDL: The hostname to check. This argument should be in the origin URL format, with no pathname. For example: "https://www.site.com".

This comment has been minimized.

@6a68

6a68 Feb 8, 2019

Member

Actually, looking at the schema docs, I haven't been super strict about reusing the IDL descriptions. This could be fine as is (but aHost does bug me ;-P).

{
"name": "setLoginSavingEnabled",
"type": "function",
"description": "For the given origin, uses the given value to set a boolean flag which determines whether login saving is enabled",

This comment has been minimized.

@6a68

6a68 Feb 8, 2019

Member

The IDL docs may be clearer here. Also, be sure to mention cases in which the returned promise is rejected, which I think is just if the URL is malformed.

throw new ExtensionError(ex);
}
},
setLoginSavingEnabled(aHost, isEnabled) {

This comment has been minimized.

@6a68

6a68 Feb 8, 2019

Member

How about origin and shouldSaveLogins for the arguments?

This comment has been minimized.

@lmorchard

lmorchard Feb 8, 2019

Author Contributor

Yeah, same as above, I was trying to follow the underlying API rather than invent new names

const enableRememberSignons = async () => {
await webext.inChrome();
await driver.executeScript(`
Services.prefs.setBoolPref("signon.rememberSignons", true);

This comment has been minimized.

@6a68

6a68 Feb 8, 2019

Member

It would be good to actually call the Services.logins.get/setLoginSavingEnabled for a specific origin, then test it's correctly returned when you call the API. I wonder what happens if you accidentally leave in a trailing slash, for instance. This could be left for a followup commit.

await disableRememberSignons();
});
it("does not enable login saving for origin when given true", async () => {
await commonTest(true, false);

This comment has been minimized.

@6a68

6a68 Feb 8, 2019

Member

Good catch! I'm not sure which, if any, parts of the logins API will work if logins are globally disabled. I think this needs investigation; could you file a bug?

@6a68

This comment has been minimized.

Copy link
Member

6a68 commented Feb 8, 2019

Whoops! Didn't see Matt had already reviewed. Sorry to pile on :-P

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

lmorchard commented Feb 8, 2019

I added a commit that automatically re-adds our entry to the exception list for login saving when it is somehow removed (manually or otherwise). However, this still needs unit / integration tests. However, however, this could use a sanity check as to whether this is what we actually want to do.

Also: Adding WIP because I do intend to add unit / integration tests for the additions in the new commit

@lmorchard lmorchard force-pushed the lmorchard:57-disable-login-saving branch from d253c8c to 59c1ce4 Feb 8, 2019

@lmorchard lmorchard changed the title Disable prompts to save logins while on extension pages WIP: Disable prompts to save logins while on extension pages Feb 8, 2019

Disable prompts to save logins while on extension pages
- Add getLoginSavingEnabled and setLoginSavingEnabled methods to the
  Logins experiment API

- Disable login saving for extension pages on background startup

Fixes #57

@lmorchard lmorchard force-pushed the lmorchard:57-disable-login-saving branch from 59c1ce4 to a6efbfa Feb 11, 2019

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

lmorchard commented Feb 11, 2019

Actually, you know what: Never mind my last commit. It occurred to me over the weekend that this PR without the event handler to automatically re-enable the exception was already 80% of the solution and pretty much reviewed.

My commit that uses the permission events still needs time to bake, so I'll do it in a follow-up PR. I can probably add some more test cases there, too

@lmorchard lmorchard changed the title WIP: Disable prompts to save logins while on extension pages Disable prompts to save logins while on extension pages Feb 11, 2019

@lmorchard lmorchard merged commit 10feb59 into mozilla-lockbox:master Feb 11, 2019

3 checks passed

codecov/patch 100% of diff hit (target 50%)
Details
codecov/project 95.9% (+<.1%) compared to 29c1fd9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Feb 11, 2019

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