Display permission prompt for webextensions background updates

VERIFIED FIXED in Firefox 53

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: aswan, Assigned: aswan)

Tracking

51 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 verified)

Details

(Whiteboard: triaged)

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8810567 [details]
update indicator mock
(Assignee)

Comment 2

2 years ago
Created attachment 8810569 [details]
update menu mock
(Assignee)

Comment 3

2 years ago
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?
Flags: needinfo?(mjaritz)

Comment 4

2 years ago
Hi Markus, any comments on Andrews' questions below
Whiteboard: [permissions, UI needed]
(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.
Flags: needinfo?(mjaritz)
Whiteboard: [permissions, UI needed]
(Assignee)

Comment 6

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

Updated

2 years ago
Whiteboard: triaged
(Assignee)

Updated

2 years ago
Assignee: nobody → aswan
Priority: -- → P1
(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.
Flags: needinfo?(mjaritz)
(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)
Flags: needinfo?(mjaritz)
(Assignee)

Comment 9

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

2 years ago
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.
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.
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.
(Assignee)

Comment 13

2 years ago
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)
(Assignee)

Updated

2 years ago
Summary: Display permission prompt for webextensions updates → Display permission prompt for webextensions background updates
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8826418 - Flags: review?(florian)

Comment 15

2 years ago
mozreview-review
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?
Attachment #8826418 - Flags: review?(florian)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
mozreview-review-reply
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.
(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?
> 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.
(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

2 years ago
mozreview-review
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 ':'
Attachment #8826418 - Flags: review?(florian) → review+
> > 
> > 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.
(Assignee)

Comment 23

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

Comment 27

2 years ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79d111e76fc9
Show permission prompts for background webextension updates r=florian
Comment hidden (mozreview-request)

Comment 30

2 years ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc024a9abc96
Show permission prompts for background webextension updates r=florian

Comment 31

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc024a9abc96
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

2 years ago
Flags: needinfo?(aswan)
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?
Flags: needinfo?(aswan)
(Assignee)

Comment 33

2 years ago
(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?
Flags: needinfo?(aswan)

Updated

2 years ago
Depends on: 1334746
(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.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
You need to log in before you can comment on or make changes to this bug.