1298630 - New exception setting is gone for install add-ons after restarting

Status: NEW

(Core :: Permission Manager, defect, P3)

Description

[magicp]

Attachment: exception-setting-is-gone-for-install-addons.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160827030436

Steps to reproduce:

a-1. Start Nightly with new profile
a-2. Go to "https://addons.mozilla.org" and check the site info icon
a-3. Open menu > Options > Security
a-4. Click [Exceptions...] button in General
a-5. Confirm allowed sites for install add-ons
     "https://addons.mozilla.org" "Allow"
     "https://testpilot.firefox.org" "Allow"
a-6. Remove All Sites
a-7. Save Changes
a-8. Close Options

b-1. Restart Nightly
b-2. Go to "https://addons.mozilla.org" and check the site info icon
b-3. Open menu > Options > Security
b-4. Click [Exceptions...] button in General
b-5. Confirm allowed sites for install add-ons
      (Nothing)
b-6. Add allowed site "https://addons.mozilla.org"
b-7. Save Changes
b-8. Close Options

c-1. Restart Nightly
c-2. Go to "https://addons.mozilla.org" and check the site info icon
c-3. Open menu > Options > Security
c-4. Click [Exceptions...] button in General
c-5. Confirm allowed sites for install add-ons


Actual results:

In step c-5,  "https://addons.mozilla.org" is gone, and also "https://testpilot.firefox.org" is coming on its own.


Expected results:

The exception settings works correctly.

Comment 1

[magicp]

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5bd6e09f074e&tochange=afc933adf723

Comment 2

[Julien Cristau [:jcristau] (back April 22)]

Too late for firefox 52, mass-wontfix.

Comment 3

[Daniel Veditz [:dveditz]]

In that regression range probably Bug 1050080. Any ideas, Mark?

Comment 4

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

Vlad, can you check if this issue still happens on release?

Comment 5

[Vlad Jiman]

Hello, I've verified using the above steps on: FF 51, Nightly 64.0a1 and the release version 62.0.3. On all of these the issue appears to reproduce, given that we remove the exception and upon restarting a link "https://testpilot.firefox.org" is added, also in some cases if we save the exception, upon restart it will be gone, I've attached videos of each of these problems. Tests were performed using Windows 10 x64 and new FF profiles on each version.

Comment 6

[Vlad Jiman]

Attachment: testpilot.firefox.com link appears

Comment 7

[Vlad Jiman]

Attachment: Saving a new exception

Comment 8

[Andrew Swan [:aswan]]

I'm able to reproduce this but only inconsistently.  In any case, I think its an issue with site permissions, I think this is the right component for those.

Comment 9

[Johann Hofmann [:johannh]]

Yeah, this could be a bug in the import code, probably needs a lot deeper investigation.

Andrew, there's something else I noticed. addons.mozilla.org and testpilot.firefox.com seem to be able to install add-ons just fine without the "install" permission for me. Is that permission still needed?

Comment 10

[Andrew Swan [:aswan]]

(In reply to Johann Hofmann [:johannh] from comment #9)
> Andrew, there's something else I noticed. addons.mozilla.org and
> testpilot.firefox.com seem to be able to install add-ons just fine without
> the "install" permission for me. Is that permission still needed?

Both those sites have access to two different APIs: InstallTrigger which is controlled by site permissions, and mozAddonManager which is not.  It sounds like both sites have moved over to using mozAddonManager instead of InstallTrigger.  Perhaps we should remove the default xpinstall site permissions if those sites aren't using InstallTrigger any more.  Jared, Stuart, any objection?

One note, there are some (undocumented) preferences that users can flip that disable mozAddonManager and these sites might use InstallTrigger as a fallback in that case, but I don't think we need to worry about it -- InstallTrigger will still work but with one extra propmt.

Comment 11

[[e-mail]]

Once the shield study on the disco pane concludes (Oct 29th) we'll be switching to the unified mozAddonManager powered button that's already in use on AMO. When that happens all install buttons across AMO and disco will have a direct install of the XPI as a fallback (no InstallTrigger).

Comment 12

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Test Pilot relies on mozAddonManager only. This is the first I've even heard of InstallTrigger.

Comment 13

[Johann Hofmann [:johannh]]

Thank you!

(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #11)
> Once the shield study on the disco pane concludes (Oct 29th) we'll be
> switching to the unified mozAddonManager powered button that's already in
> use on AMO. When that happens all install buttons across AMO and disco will
> have a direct install of the XPI as a fallback (no InstallTrigger).

So that means we can remove this permission after October 29th? Is there a bug for the shield study that we can link this bug to?

Comment 14

[[e-mail]]

(In reply to Johann Hofmann [:johannh] from comment #13)
> Thank you!
> 
> (In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #11)
> > Once the shield study on the disco pane concludes (Oct 29th) we'll be
> > switching to the unified mozAddonManager powered button that's already in
> > use on AMO. When that happens all install buttons across AMO and disco will
> > have a direct install of the XPI as a fallback (no InstallTrigger).
> 
> So that means we can remove this permission after October 29th? Is there a
> bug for the shield study that we can link this bug to?

Not directly, but if you follow this issue that's should be close enough https://github.com/mozilla/addons-frontend/issues/6539

Comment 15

[Johann Hofmann [:johannh]]

Stuart, sorry to needinfo you again, but do we have confirmation now that the InstallTrigger permissions are no longer needed?

Comment 16

[[e-mail]]

(In reply to Johann Hofmann [:johannh] from comment #15)
> Stuart, sorry to needinfo you again, but do we have confirmation now that
> the InstallTrigger permissions are no longer needed?

The buttons on disco pane are now live, so the only place that InstallTrigger continues to exist is the old AMO frontend. Here: https://github.com/mozilla/addons-server/blob/dce35f1d21380c806855ae9496dc828b190cc3a3/static/js/zamboni/buttons.js#L309

Unfortunately we're not yet ready to remove the old frontend (It's likely to happen in Q1 next year though - so would you be able to wait until then?) If not we'd need to remove installTrigger from the old frontend and start using mozAddonManager there too to avoid breaking people. Or alternatively switch to a link which would mostly work except cases where MITM proxies are used by end users since that breaks installs as the MITM cert isn't trusted for add-on installs. However, getting people to use the new frontend as a workaround might be ok, though I'd have to check with product for that.

Comment 17

[Johann Hofmann [:johannh]]

(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #16)
> (In reply to Johann Hofmann [:johannh] from comment #15)
> > Stuart, sorry to needinfo you again, but do we have confirmation now that
> > the InstallTrigger permissions are no longer needed?
> 
> The buttons on disco pane are now live, so the only place that
> InstallTrigger continues to exist is the old AMO frontend. Here:
> https://github.com/mozilla/addons-server/blob/
> dce35f1d21380c806855ae9496dc828b190cc3a3/static/js/zamboni/buttons.js#L309
> 
> Unfortunately we're not yet ready to remove the old frontend (It's likely to
> happen in Q1 next year though - so would you be able to wait until then?) If
> not we'd need to remove installTrigger from the old frontend and start using
> mozAddonManager there too to avoid breaking people. Or alternatively switch
> to a link which would mostly work except cases where MITM proxies are used
> by end users since that breaks installs as the MITM cert isn't trusted for
> add-on installs. However, getting people to use the new frontend as a
> workaround might be ok, though I'd have to check with product for that.

Is the old frontend still used, then? If so then we can just block this bug on that. Otherwise (if there are no users, you just haven't removed it yet) it's fine to remove the permission since that shouldn't actually break the API, it just adds an additional permission prompt in front of it, AFAIK.

Comment 18

[[e-mail]]

(In reply to Johann Hofmann [:johannh] from comment #17)
> (In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #16)
> > (In reply to Johann Hofmann [:johannh] from comment #15)
> > > Stuart, sorry to needinfo you again, but do we have confirmation now that
> > > the InstallTrigger permissions are no longer needed?
> > 
> > The buttons on disco pane are now live, so the only place that
> > InstallTrigger continues to exist is the old AMO frontend. Here:
> > https://github.com/mozilla/addons-server/blob/
> > dce35f1d21380c806855ae9496dc828b190cc3a3/static/js/zamboni/buttons.js#L309
> > 
> > Unfortunately we're not yet ready to remove the old frontend (It's likely to
> > happen in Q1 next year though - so would you be able to wait until then?) If
> > not we'd need to remove installTrigger from the old frontend and start using
> > mozAddonManager there too to avoid breaking people. Or alternatively switch
> > to a link which would mostly work except cases where MITM proxies are used
> > by end users since that breaks installs as the MITM cert isn't trusted for
> > add-on installs. However, getting people to use the new frontend as a
> > workaround might be ok, though I'd have to check with product for that.
> 
> Is the old frontend still used, then? If so then we can just block this bug
> on that. Otherwise (if there are no users, you just haven't removed it yet)
> it's fine to remove the permission since that shouldn't actually break the
> API, it just adds an additional permission prompt in front of it, AFAIK.

We default to the new site but the old site is available optionally. I don't think having an additional dialogue should be a problem but Product (:jorgev) should have input on that.

Comment 19

[Jorge Villalobos [:jorgev] (he/him)]

That should be fine for users of the old front-end.