1295151 - Install add-on should not show as a permission in the control center

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Ash Grigas]

Please remove this - only access is in the Tools page info panel.

Comment 1

[Matthew N. [:MattN]]

We already discussed this in bug 1271868 comment 8 and on and decided against it. What has changed?

If an add-on grants this permission to evil.com why shouldn't I be in control of this and be able to revoke from control center?

Comment 2

[Ash Grigas]

(In reply to Matthew N. [:MattN] from comment #1)
> We already discussed this in bug 1271868 comment 8 and on and decided
> against it. What has changed?
> 
> If an add-on grants this permission to evil.com why shouldn't I be in
> control of this and be able to revoke from control center?

No we decided for cookies for a user that manually goes to the Tools menu and blocks cookies.

For this case, we never ask users outside of the add-on install flow if they want to allow the site to install the add-on, they just go through the install flow. If a user clicks the X in the permissions dropdown, its confusing since we don't reprompt the decision - do you want to allow X site to install an add-on. It then makes the user feel like somethings broken since there is no clear way how to change their mind/allow the site to install an add-on. I think there is most value to the user making a decision at the time of installing the add-on, not after the fact for future add-ons.

If a user explicitly wants to block the site from installing future add-ons for the site by going to the Tools menu, I'm open to showing that it was blocked. I don't think it should appear in the control panel by default (allow add-ons to be installed).

Comment 3

[Matthew N. [:MattN]]

(In reply to agrigas from comment #2)
> (In reply to Matthew N. [:MattN] from comment #1)
> > We already discussed this in bug 1271868 comment 8 and on and decided
> > against it. What has changed?
> > 
> > If an add-on grants this permission to evil.com why shouldn't I be in
> > control of this and be able to revoke from control center?
> 
> No we decided for cookies for a user that manually goes to the Tools menu
> and blocks cookies.

I'm not sure what you mean since that bug was about cookies, images and add-ons and more generally about having all non-default values there.

> For this case, we never ask users outside of the add-on install flow if they
> want to allow the site to install the add-on, they just go through the
> install flow. If a user clicks the X in the permissions dropdown, its
> confusing since we don't reprompt the decision - do you want to allow X site
> to install an add-on.

Yes, we do, you just get an additional scarier doorhanger before the real doorhanger.

> It then makes the user feel like somethings broken
> since there is no clear way how to change their mind/allow the site to
> install an add-on. I think there is most value to the user making a decision
> at the time of installing the add-on, not after the fact for future add-ons.

They can allow the installation without allowing the site for the future.

> If a user explicitly wants to block the site from installing future add-ons
> for the site by going to the Tools menu, I'm open to showing that it was
> blocked. I don't think it should appear in the control panel by default
> (allow add-ons to be installed).

OK, so taking a step back, is the permissions section supposed to show "granted/denied permissions" or "non-default permissions"? Currently it's the former yet the only action (x) is supposed to revert to the default IIUC. It seems like Control Center should only show non-default permissions and the (x) reverts to the default. i.e. the "allow" for addons.mozilla.org shouldn't show but "allow" for example.com or "deny" for addons.mozilla.org should appear?

Comment 4

[Ash Grigas]

(In reply to Matthew N. [:MattN] from comment #3)
> (In reply to agrigas from comment #2)
> > (In reply to Matthew N. [:MattN] from comment #1)
> > > We already discussed this in bug 1271868 comment 8 and on and decided
> > > against it. What has changed?
> > > 
> > > If an add-on grants this permission to evil.com why shouldn't I be in
> > > control of this and be able to revoke from control center?
> > 
> > No we decided for cookies for a user that manually goes to the Tools menu
> > and blocks cookies.
> 
> I'm not sure what you mean since that bug was about cookies, images and
> add-ons and more generally about having all non-default values there.
> 
> > For this case, we never ask users outside of the add-on install flow if they
> > want to allow the site to install the add-on, they just go through the
> > install flow. If a user clicks the X in the permissions dropdown, its
> > confusing since we don't reprompt the decision - do you want to allow X site
> > to install an add-on.
> 
> Yes, we do, you just get an additional scarier doorhanger before the real
> doorhanger.
> 
> > It then makes the user feel like somethings broken
> > since there is no clear way how to change their mind/allow the site to
> > install an add-on. I think there is most value to the user making a decision
> > at the time of installing the add-on, not after the fact for future add-ons.
> 
> They can allow the installation without allowing the site for the future.
> 
> > If a user explicitly wants to block the site from installing future add-ons
> > for the site by going to the Tools menu, I'm open to showing that it was
> > blocked. I don't think it should appear in the control panel by default
> > (allow add-ons to be installed).
> 
> OK, so taking a step back, is the permissions section supposed to show
> "granted/denied permissions" or "non-default permissions"? Currently it's
> the former yet the only action (x) is supposed to revert to the default
> IIUC. It seems like Control Center should only show non-default permissions
> and the (x) reverts to the default. i.e. the "allow" for addons.mozilla.org
> shouldn't show but "allow" for example.com or "deny" for addons.mozilla.org
> should appear?

The permissions section should not just reflect the defaults - that was the old model that like in the Page Info showed the users every possible permission regardless of it was something they should pay attention to and regardless of if they participated in the grant by allowing or blocking the permission. When they user interacts with the notification prompt that are more likely to connect the control center contents with the question they were asked since it was a conscious allow or block on their part.

Control center should show permissions that user's are asked by Firefox to make a decision about - which means permissions that are set to ask by default. X essentially is supposed to then ask the user again - so revert those 'ask' permissions back to the default. 

The outliers are popup blocking and plugins since those have some non-standard use cases and firefox is taking action for the user for security reasons most of the time and so we don't bother asking the user the prompt.

Comment 5

[Ash Grigas]

The Non-AMO addons we automatically allow always for the user so there is no reversing that action. There is no always block so the X is useless.

For third party sites there is only a one time allow and a not now as options so there is no really block action that is supported and therefore the X doesn't have real user value for that use case either. This isn't really a permission even if its implemented like the others are.

Comment 6

[Matthew N. [:MattN]]

(In reply to agrigas from comment #4)
> The permissions section should not just reflect the defaults - that was the
> old model that like in the Page Info showed the users every possible
> permission regardless of it was something they should pay attention to and
> regardless of if they participated in the grant by allowing or blocking the
> permission.

I don't follow this sentence. Having the permissions section only show permissions that are not set to the application default is not at all like Page Info since Page Info showed all permissions. I'm proposing we only show non-default permissions which means the user "participated in the grant by allowing or blocking the permission."

> When they user interacts with the notification prompt that are
> more likely to connect the control center contents with the question they
> were asked since it was a conscious allow or block on their part.

> Control center should show permissions that user's are asked by Firefox to
> make a decision about - which means permissions that are set to ask by
> default. X essentially is supposed to then ask the user again - so revert
> those 'ask' permissions back to the default. 

OK, so it sounds like we agree to switch from Control Center showing "granted/denied permissions" to showing "non-default permissions".

(In reply to agrigas from comment #5)
> The Non-AMO addons we automatically allow always for the user so there is no
> reversing that action. There is no always block so the X is useless.

It wouldn't be useless. The (X) means "revert to default" in my mind so the (X) button would revert to the application default which means to remove the site from the allow list.

> For third party sites there is only a one time allow and a not now as
> options so there is no really block action that is supported and therefore
> the X doesn't have real user value for that use case either. This isn't
> really a permission even if its implemented like the others are.

Note that users can Allow permanently from about:preferences#security and an add-on can do that programmatically.

Comment 7

[Johann Hofmann [:johannh]]

[Tracking Requested - why for this release]: Bug 1203292 introduced the (x) icons in the control center, enabling users to clear the addon install permission for addons.mozilla.org without a way to undo. If we decide that we don't want them to be able to do this, we need to uplift to 50 before a ton of users takes this action.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jared, since you r+'d bug 1203922, could you please help find an owner? Thanks!

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Ritu Kothari (:ritu) from comment #8)
> Hi Jared, since you r+'d bug 1203922, could you please help find an owner?
> Thanks!

I meant  Bug 1203292.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Fred, could you resurrect your patch from bug 1271868 and have it only filter out the add-on install permission for now?

Comment 11

[Matthew N. [:MattN]]

(In reply to Panos Astithas [:past] from comment #10)
> Fred, could you resurrect your patch from bug 1271868 and have it only
> filter out the add-on install permission for now?

That doesn't seem to align with the plan that agrigas and I were converging on which is to only show non-"default value" permissions in control center. If you want to do this for 50 I guess it's okay but I still think it's not the right model for Control Center. I filed bug 1303108 for the revert to default proposal.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Yes, we need this in 50 and it is already very late. We'll discuss the long-term bug in planning next week.

Comment 13

[[e-mail]]

Ok I'll make a temp fix for aurora and send review to :MattN

Comment 14

[[e-mail]]

Attachment: Bug 1295151 - Install add-on should not show as a permission in the control center;

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

Comment 15

[[e-mail]]

The related implementation is changed from bug 1271868. Since `getPermissionDetailsByURI` is called only in browser.js, I'd take a simpler approach (with test) for this workaround (which is only applied to aurora)

https://dxr.mozilla.org/mozilla-central/search?q=getPermissionDetailsByURI&redirect=false

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8792373 [details]
Bug 1295151 - Install add-on should not show as a permission in the control center;

https://reviewboard.mozilla.org/r/79440/#review78018

Please check if this also removes the dot on the (i) icon too.

::: browser/modules/SitePermissions.jsm:64
(Diff revision 1)
> +      // XXX Bug 1295151 - Install add-on should not show as a permission
> +      // in the control center

I think it's preferred to use comments with open bugs with the future fix so I think referencing bug 1303108 would be better.

::: browser/modules/SitePermissions.jsm:66
(Diff revision 1)
> +      if (id != "install") {
> -      let availableStates = this.getAvailableStates(id).map( state => {
> +        let availableStates = this.getAvailableStates(id).map( state => {
> -        return { id: state, label: this.getStateLabel(id, state) };
> +          return { id: state, label: this.getStateLabel(id, state) };
> -      });
> +        });
> -      let label = this.getPermissionLabel(id);
> +        let label = this.getPermissionLabel(id);
>  
> -      permissions.push({id, label, state, availableStates});
> +        permissions.push({id, label, state, availableStates});
> -    }
> +      }
> +    }

It's preferred to avoid changing the indentation and using a `continue` in this case. Just like early returns are preferred in other cases.

Comment 17

[[e-mail]]

Comment on attachment 8792373 [details]
Bug 1295151 - Install add-on should not show as a permission in the control center;

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

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Fred Lin [:gasolin] from comment #15)
> `getPermissionDetailsByURI` is called only in browser.js, I'd take a simpler
> approach (with test) for this workaround (which is only applied to aurora)

To be clear, I think we should land the fix in both m-c and aurora (even beta if this doesn't make the cut today). We want to do something better in bug 1303108, but I don't want us to scramble at the last minute again a few weeks from now if a fix there takes a while to materialize.

Comment 19

[[e-mail]]

Thanks for help! 

The new patch will not show the dot on the (i) icon when visiting `addon.mozilla.org`, addressed issues and it's based on master now. I'll attach aurora patch once its r+.

The `getAllByURI` method is called only in browser.js. 
https://dxr.mozilla.org/mozilla-central/search?q=getAllByURI&redirect=false

It might worth to change the method name since I exclude the `install` permission from it.

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8792373 [details]
Bug 1295151 - Install add-on should not show as a permission in the control center;

https://reviewboard.mozilla.org/r/79440/#review78046

Comment 21

[[e-mail]]

thanks!

Comment 22

[[e-mail]]

Attachment: 0001-Bug-1295151-Install-add-on-should-not-show-as-a-perm.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1295151
[User impact if declined]: When user visit addon.mozilla.org , they will able to change install addon permission in control center 
[Describe test coverage new/current, TreeHerder]: green
[Risks and why]: late patch
[String/UUID change made/needed]: N

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20c898adf320
Install add-on should not show as a permission in the control center; r=MattN

Comment 24

[Matthew N. [:MattN]]

Fred, regarding your try push, marionette isn't used very much for browser UI testing. mochitest-browser-chrome is more useful so I would recommend it for the future.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8792428 [details] [diff] [review]
0001-Bug-1295151-Install-add-on-should-not-show-as-a-perm.patch

50 moved to Beta today.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8792428 [details] [diff] [review]
0001-Bug-1295151-Install-add-on-should-not-show-as-a-perm.patch

Please re-nominate once the fix has landed on Nightly (m-c)

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/20c898adf320

Comment 28

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 52.0a1 (2016-09-20) Win 7, Ubuntu 14.04, OS X 10.11.

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8792428 [details] [diff] [review]
0001-Bug-1295151-Install-add-on-should-not-show-as-a-perm.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1295151
[User impact if declined]: When users visit addons.mozilla.org, they will able to change install addon permission in control center 
[Describe test coverage new/current, TreeHerder]: green
[Risks and why]: late patch
[String/UUID change made/needed]: N

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8792428 [details] [diff] [review]
0001-Bug-1295151-Install-add-on-should-not-show-as-a-perm.patch

Fix was verified on Nightly52, Aurora51+, Beta50+

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8038403f9988

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b77914e016e3

Comment 33

[Andrei Vaida [:avaida]]

Paul, could you please take a look at this fix on 51 and 50 as well?

Comment 34

[Andreas Tolfsen ❲:ato❳]

(In reply to Matthew N. [:MattN] (In Taipei until Sep. 23) from comment #24)
> Fred, regarding your try push, marionette isn't used very much for browser
> UI testing. mochitest-browser-chrome is more useful so I would recommend it
> for the future.

That is factually not true.

Marionette is being used for the Firefox UI test suite that gets triggered on try pushes, which replaces the old mozmill tests.

Whether to write a test using mochitest-browser-chrome or Fx-UI/Marionette depends on what you’re testing.  The former gives a tighter integration with chrome JS for when you need that, whereas the latter is closer to a functional test in the way a user would interact with the browser.

Comment 35

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 50b2, 51.0a2 (2016-09-26), Win 7 x64.

Comment 36

[Paul Zühlcke [:pbz]]

I've submitted a patch for Bug 1372033 which changes addon installation permissions to the default behaviour. Thus, the fix here is no longer needed. My patch reverts it.