1317470 - Display permission prompt for webextensions background updates

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Andrew Swan [:aswan]]

When a webextension is updated and the new version requires different permissions than the old version, we need to notify the user and have them accept the new permissions.  The UX design is similar to the sideloading flow tracked in bug 1317363.  This bug is to implement the specified flow.

Comment 1

[Andrew Swan [:aswan]]

Attachment: update indicator mock

Comment 2

[Andrew Swan [:aswan]]

Attachment: update menu mock

Comment 3

[Andrew Swan [:aswan]]

A few questions:
1. Should the badge used here be the same one we use for sideloading or something different?
2. What should happen if there are multiple updates available at the same time?  Should there be one item in the menu for all updates or a separate item per update?
3. I thought we talked about jumping to the details page in the add-ons manager for the extension in question when the user selects the menu item about the update since the currently specified modal dialog isn't feasible?
4. The flow on figma still has language about disabling the original extension until the user accepts the update, I thought we agreed not to do that?

Comment 4

[:shell escalante]

Hi Markus, any comments on Andrews' questions below

Comment 5

[Markus Jaritz [:designakt] (UX)]

(In reply to Andrew Swan [:aswan] from comment #3)
> A few questions:
> 1. Should the badge used here be the same one we use for sideloading or
> something different?
It should be the same.

> 2. What should happen if there are multiple updates available at the same
> time?  Should there be one item in the menu for all updates or a separate
> item per update?
One item per update. (As we do not expect many to happen at the same time.)

> 3. I thought we talked about jumping to the details page in the add-ons
> manager for the extension in question when the user selects the menu item
> about the update since the currently specified modal dialog isn't feasible?
I intended to jump to the extensions list in about:addons with the extension in question in focus. And then open the same doorhanger that we use on install. (with different copy) - shown in the flow doc and shared in our meeting. (If we have a different understanding here, let's chat.)

> 4. The flow on figma still has language about disabling the original
> extension until the user accepts the update, I thought we agreed not to do
> that?
That is because I am under the impression that we agreed on that flow. (I couldn't find anything on it in the meeting notes.)
The not-disable flow will default to leaving many users on outdated versions of extensions. (Or would need to annoy users repeatedly to update. Something we wouldn't want to support. And the disabling can be use as a tool to filter out extensions people do not care enough about to click once.

Comment 6

[Andrew Swan [:aswan]]

Thanks for the answers.  Re #3, I recall you showing updated flows that use about:addons and the doorhanger, but that's not what I see on figma, am I looking in the wrong place?
Re #4, my recollection is the opposite.  As a user of privacy related extensions (e.g., noscript, https everywhere, privacy badger, etc.) I would be upset if any of those extensions was disabled without my consent for any period of time.

Comment 7

[Markus Jaritz [:designakt] (UX)]

(In reply to Andrew Swan [:aswan] from comment #6)
> Thanks for the answers.  Re #3, I recall you showing updated flows that use
> about:addons and the doorhanger, but that's not what I see on figma, am I
> looking in the wrong place?
Use this link to be sure to see v1 of the Permssions flows:
https://www.figma.com/file/HrLiKUwoLQZsIUIVBKM8Wnnu/Install-Flow-showing-Permissions?node-id=321%3A78

> Re #4, my recollection is the opposite.  As a user of privacy related
> extensions (e.g., noscript, https everywhere, privacy badger, etc.) I would
> be upset if any of those extensions was disabled without my consent for any
> period of time.
This argument is new to me. I will evaluate and get back to you.

Comment 8

[Markus Jaritz [:designakt] (UX)]

(In reply to Andrew Swan [:aswan] from comment #6)
> Re #4, my recollection is the opposite.  As a user of privacy related
> extensions (e.g., noscript, https everywhere, privacy badger, etc.) I would
> be upset if any of those extensions was disabled without my consent for any
> period of time.

Those users will be notified about our action as soon as they start Firefox. That means that Firefox starts a new session, so nothing privacy related should happen. - Not a perfect solution for very privacy aware users, but the best for most, as it does not leave them on outdated versions of extensions.(see comment 5)

Comment 9

[Andrew Swan [:aswan]]

(In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #8)
> Those users will be notified about our action as soon as they start Firefox.
> That means that Firefox starts a new session, so nothing privacy related
> should happen. - Not a perfect solution for very privacy aware users, but
> the best for most, as it does not leave them on outdated versions of
> extensions.(see comment 5)

I'm not sure what you mean by "nothing privacy related should happen"?  When the browser restarts, it will immediately reload open tabs from the previous session.  And if I use noscript or disconnect or something that limits what the browser loads while it opens tabs, I would be very upset if it was disabled when it opened those tabs without those extensions in place.

I don't object to encouraging users to run the latest versions of extensions, but I think this method of doing it goes much too far.

Comment 10

[Andy McKay]

I also was under the impression that we'd agreed not to disable add-ons, because we wouldn't want to surprise a user and disable add-on that provided a security or privacy benefit. In this case having users on an older version feels the lesser of two evils.

Comment 11

[Kev Needham [:kev]]

I also don't remember the decision being taken that we'd disable extensions on a permissions update. 

I thought we had agreed on notification only, to keep users in control of their addons, and to allow them to continue to use an older addon in the event that they didn't agree to changes to permissions. This use case is a real possibility if an update asks for additional permissions the user is unwilling to grant. We can continue to make the user aware of the newer version, but agree we shouldn't disable an addon in the event the privs change on an update, rather we should notify and not automatically update as we normally would.

Comment 12

[krupa raj [:krupa--use this to needinfo]]

In the add-ons manager, the user is notified in two different ways about a pending update. 

- In the extensions tab, there is a "An Update is available" message. See https://www.dropbox.com/s/qkw4l1orkagaqf0/Screenshot%202017-01-09%2015.26.47.png?dl=0

- There is an additional tab called 'Available Updates' present when the user is not on the latest versions of all currently installed add-ons. See https://www.dropbox.com/s/g0iwed0l8jzbsgd/Screenshot%202017-01-09%2015.26.05.png?dl=0

Both these places should indicate if the permissions related to a webextension change in between versions. I'd suggest that we support this in v1 since the add-ons manager is where the user goes to get all information about a webextension.

Comment 13

[Andrew Swan [:aswan]]

In both the cases described in comment 12, we know there is a new version available but have not yet downloaded it, so we don't know if there are any new permissions or what they are.  Which means that without making pretty big changes, we can't display any new permissions on those pages (though I'm pretty sure that the only way to reach those two screens is by disabling background update checks so we're talking about a small minority of users).
In any case, my first patch for this bug didn't do anything special for updates applied interactively from these screens (i.e., applying an update that requires new permissions would just cause the indicator to show up on the hamburger menu).  This case wasn't detailed in the mocks but I'm assuming we just want to display the permissions notification immediately if the user is manually asking to apply the update.  I'll fix that before getting the patch up for review which I hope to do this week (it is stacked on top of a bunch of other patches that are still awaiting review)

Comment 14

[Andrew Swan [:aswan]]

Attachment: Bug 1317470 Show permission prompts for background webextension updates

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

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8826418 [details]
Bug 1317470 Show permission prompts for background webextension updates

https://reviewboard.mozilla.org/r/104364/#review105742

Looks reasonable; it'll likely be r+ once the patch this is based on is also r+.

::: browser/base/content/browser-addons.js:513
(Diff revision 1)
>        container.appendChild(button);
>      }
> +
> +    for (let update of updates) {
> +      let button = document.createElement("toolbarbutton");
> +      button.setAttribute("label", `"${update.addon.name}" requires new permissions`);

add a comment saying this needs fixing in bug NNNNNNN please.

::: browser/base/content/test/general/browser_extension_update.js:1
(Diff revision 1)
> +

nit: remove empty line here

::: browser/base/content/test/general/browser_extension_update.js:49
(Diff revision 1)
> +
> +function promiseViewLoaded(tab, viewid) {
> +  let win = tab.linkedBrowser.contentWindow;
> +  if (win.gViewController && !win.gViewController.isLoading &&
> +      win.gViewController.currentViewId == viewid) {
> +        return Promise.resolve();

fix indent please

::: browser/base/content/test/general/browser_extension_update.js:137
(Diff revision 1)
> +  ]});
> +
> +  // Install version 1.0 of the test extension
> +  let url1 = `http://localhost:${server.identity.primaryPort}/browser_webext_update1.xpi`;
> +  let install = yield AddonManager.getInstallForURL(url1, null, "application/x-xpinstall");
> +  isnot(install, null, "Created install");

ok(install,

::: browser/base/content/test/general/browser_extension_update.js:148
(Diff revision 1)
> +      },
> +    });
> +    install.install();
> +  });
> +
> +  isnot(addon, null, "Addon was installed");

same here

::: browser/base/content/test/general/browser_extension_update.js:149
(Diff revision 1)
> +    });
> +    install.install();
> +  });
> +
> +  isnot(addon, null, "Addon was installed");
> +  isnot(getBadgeStatus(), "addon-alert", "Should not start out with an addon alert badge");

Should we check the status, instead of checking it's not "addon-alert"?

::: browser/base/content/test/general/browser_extension_update.js:197
(Diff revision 1)
> +  yield PanelUI.show();
> +  addons = document.getElementById("PanelUI-footer-addons");
> +  is(addons.children.length, 0, "Update menu entries should be gone");
> +  yield PanelUI.hide();
> +
> +  // Re-check for an update

Are we going to re-prompt users each time we check for updates? (that's daily, isn't it?)

::: browser/base/content/test/general/browser_extension_update.js:230
(Diff revision 1)
> +  updatePromise = promiseUpgrade(addon);
> +  panel = yield popupPromise;
> +  panel.button.click();
> +
> +  addon = yield updatePromise;
> +  is(addon.version, "2.0", "Should have upgraded to the new version");

Should we check again that the update badge is gone?

::: browser/modules/ExtensionsUI.jsm:58
(Diff revision 1)
>          }
>        }
>      });
>    },
>  
> -  showSideloaded(browser, addon) {
> +  showAOM(browser, info) {

I don't find the AOM accronym explicit enough, and we don't seem to be space constrained here, so I would go for showAddonsManager.

Actually, either add a comment saying this returns a promise, or name this promiseAddonsManagerShown

::: toolkit/mozapps/extensions/AddonManager.jsm:1383
(Diff revision 1)
>  
> +  _updatePromptHandler(info) {
> +    let oldPerms = info.existingAddon.userPermissions || {hosts: [], permissions: []};
> +    let newPerms = info.addon.userPermissions;
> +
> +    // do something more complicated to compare host permissions?

Do it or include a bug number here to do it later?

Comment 16

[Andrew Swan [:aswan]]

Comment on attachment 8826418 [details]
Bug 1317470 Show permission prompts for background webextension updates

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

Comment 17

[Andrew Swan [:aswan]]

Comment on attachment 8826418 [details]
Bug 1317470 Show permission prompts for background webextension updates

https://reviewboard.mozilla.org/r/104364/#review105742

> add a comment saying this needs fixing in bug NNNNNNN please.

That's what the comment on line 493 is meant for

> Should we check the status, instead of checking it's not "addon-alert"?

You mean check its specific value?  I think it should generally be nothing, but that assumes that nothing else is going on in the browser that might set it to something else.  That seems like something that should generally be true during tests, but I think all we really care about here is that we're not displaying the addons badge....

> Are we going to re-prompt users each time we check for updates? (that's daily, isn't it?)

That's currently a topic of discussion among the UX team but yes the current implementation prompts on every check and yes the update check happens every 24 hours by default.

Comment 18

[Florian Quèze [:florian]]

(In reply to Andrew Swan [:aswan] from comment #17)

> > Should we check the status, instead of checking it's not "addon-alert"?
> 
> You mean check its specific value?  I think it should generally be nothing,
> but that assumes that nothing else is going on in the browser that might set
> it to something else.

Yes, I meant we should check this assumption.

> That seems like something that should generally be
> true during tests, but I think all we really care about here is that we're
> not displaying the addons badge....

I think we also care about ensuring there's not another test that's interfering with what you are intending to test.

> > Are we going to re-prompt users each time we check for updates? (that's daily, isn't it?)
> 
> That's currently a topic of discussion among the UX team but yes the current
> implementation prompts on every check and yes the update check happens every
> 24 hours by default.

That would make it hard for a user who wouldn't want to grant the additional permissions, but still would like to keep using the previous version of the add-on. I think I read somewhere that we wanted to support that case?

Comment 19

[emanuela [ux]]

> That would make it hard for a user who wouldn't want to grant the additional
> permissions, but still would like to keep using the previous version of the
> add-on. I think I read somewhere that we wanted to support that case?

The current implementation of the notification is not so strong: better to have a user aware of what's going on with his/her add-ons. Plus the user has already the possibility to turn automatic update off.

Comment 20

[Florian Quèze [:florian]]

(In reply to emanuela [ux team] from comment #19)
> > That would make it hard for a user who wouldn't want to grant the additional
> > permissions, but still would like to keep using the previous version of the
> > add-on. I think I read somewhere that we wanted to support that case?
> 
> The current implementation of the notification is not so strong: better to
> have a user aware of what's going on with his/her add-ons.

If the badge reappears every single day to re-request permission the user has already decided to not grant, we'll train the user to ignore the badge very quickly. Which means the next time another add-on has an update, it won't be noticed.

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 8826418 [details]
Bug 1317470 Show permission prompts for background webextension updates

https://reviewboard.mozilla.org/r/104364/#review106680

::: toolkit/mozapps/extensions/AddonManager.jsm:1383
(Diff revision 2)
>  
> +  _updatePromptHandler(info) {
> +    let oldPerms = info.existingAddon.userPermissions || {hosts: [], permissions: []};
> +    let newPerms = info.addon.userPermissions;
> +
> +    // See bug 1331769:  should we do something more complicated to

nit: remove the double space after ':'

Comment 22

[emanuela [ux]]

> > 
> > The current implementation of the notification is not so strong: better to
> > have a user aware of what's going on with his/her add-ons.
> 
> If the badge reappears every single day to re-request permission the user
> has already decided to not grant, we'll train the user to ignore the badge
> very quickly. Which means the next time another add-on has an update, it
> won't be noticed.


Sorry if I wasn't clear with my previous comment, let me try again ;)

When the user decides to not update an extension, the user is taking that decision per version, not per extension.

The badge will not reappear until there is a new version of the extension available.

Comment 23

[Andrew Swan [:aswan]]

Comment on attachment 8826418 [details]
Bug 1317470 Show permission prompts for background webextension updates

https://reviewboard.mozilla.org/r/104364/#review105742

> That's currently a topic of discussion among the UX team but yes the current implementation prompts on every check and yes the update check happens every 24 hours by default.

Filed bug 1332360 to do this as a follow-up (see detailed notes there..)

Comment 24

[Andrew Swan [:aswan]]

Comment on attachment 8826418 [details]
Bug 1317470 Show permission prompts for background webextension updates

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

Comment 25

[Andrew Swan [:aswan]]

Comment on attachment 8826418 [details]
Bug 1317470 Show permission prompts for background webextension updates

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

Comment 26

[Andrew Swan [:aswan]]

Comment on attachment 8826418 [details]
Bug 1317470 Show permission prompts for background webextension updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104364/diff/4-5/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79d111e76fc9
Show permission prompts for background webextension updates r=florian

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=70508897&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/18775df0c594

Comment 29

[Andrew Swan [:aswan]]

Comment on attachment 8826418 [details]
Bug 1317470 Show permission prompts for background webextension updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104364/diff/5-6/

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc024a9abc96
Show permission prompts for background webextension updates r=florian

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc024a9abc96

Comment 32

[Vasilica Tamas, QA [:vtamas]]

Confirm that permission pop-up is successfully displayed while a webextension is upgraded via background update to a new version which requires different permissions that the old one. Verified on Firefox 54.0a1 (2017-01-30) and Firefox 53.0a2 (2017-01-30) under Windows 10 64-bit and Ubuntu 16.04 32-bit. 

During testing I’ve noticed the following:
  - reproduced Bug 1334010 and Bug 1335333
  - there is no longer confirmation message displayed https://www.screencast.com/t/36W3adWM. We got rid of them?

Comment 33

[Andrew Swan [:aswan]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #32)
>   - there is no longer confirmation message displayed
> https://www.screencast.com/t/36W3adWM. We got rid of them?

Sorry, I'm unclear if this is about background updates or interactive updates, can you give more specific STR for this?

Comment 34

[Vasilica Tamas, QA [:vtamas]]

(In reply to Andrew Swan [:aswan] from comment #33)
> Sorry, I'm unclear if this is about background updates or interactive
> updates, can you give more specific STR for this?

My bad, I confused the scenarios. 
Background updates are done silently so there are no such messages. These are specific for interactive updates.

Based on Comment 32 I am marking this bug as Verified.