1288178 - Allow testpilot to use mozAddonManager

Status: RESOLVED FIXED

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

Description

[Andy McKay]

Bug 1287125 was getting a bit to complicated, so we thought we'd split the bug over here to make it clearer. The eventual goal is give testpilot access to the same sweet, sweet install flow that AMO and the discovery pane has.

We need to make a couple of changes for this:
* give testpilot access to mozAddonManager
* ensure that the testpilot domains mentioned in bug 1287125 comment 29 are added

I think that should be it.

Comment 1

[Wil Clouser [:clouserw]]

I see bug 1287125 was closed and uplifted.  What does the P2 priority signify for timeline here?  Is there something I can do to push this forward?

Comment 2

[:shell escalante]

for Will - P2 for us means it's not blocking 51, but a high priority.  There isn't an owner yet - so worth discussing if this is a high priority for you. 

Need info'd andy to see what the blockers for moving this to test pilot.   I know there are some security considerations that need to be factored.  the user research on the new install flow was very positive.

i wasn't sure if we wanted to fix one permissions access issue for displaying a slick drop notification on status (present in Discovery Pane) before moving to another location.

Comment 3

[Wil Clouser [:clouserw]]

Attachment: 1288128.patch

Thanks Shell.  I'm attaching a patch.  I don't do this much, so hoping it's the right format (I followed MDN's instructions).  Rob was the last reviewer so asking for r? from him this time as well.

Also, doesn't having example.com in the whitelist make anyone else a little nervous?  It doesn't look like the unit tests use it, maybe we should delete it?

Thanks!

Comment 4

[Robert Helmer [:rhelmer]]

(In reply to Wil Clouser [:clouserw] from comment #3)
> Created attachment 8779497 [details] [diff] [review]
> 1288128.patch
> 
> Thanks Shell.  I'm attaching a patch.  I don't do this much, so hoping it's
> the right format (I followed MDN's instructions).  Rob was the last reviewer
> so asking for r? from him this time as well.
> 
> Also, doesn't having example.com in the whitelist make anyone else a little
> nervous?  It doesn't look like the unit tests use it, maybe we should delete
> it?

Andrew, do you remember why we left "example.com" in WEBAPI_TEST_INSTALL_HOSTS, is that just for manual testing purposes? I remember talking about it but it must have been in IRC as it's not mentioned in the original bug.

I'm not too worried about having it here since it's just test hosts and requires the testing pref set, if someone can MITM example.com presumably they can with the others too - but we should only have it if we need it.

Comment 5

[Andrew Swan [:aswan]]

(In reply to Robert Helmer [:rhelmer] from comment #4)
> Andrew, do you remember why we left "example.com" in
> WEBAPI_TEST_INSTALL_HOSTS, is that just for manual testing purposes? I
> remember talking about it but it must have been in IRC as it's not mentioned
> in the original bug.

Its there since that's how we reach the local http server in mochitests.
References to it are a little roundabout but e.g.:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/head.js#29

Comment 6

[Robert Helmer [:rhelmer]]

Comment on attachment 8779497 [details] [diff] [review]
1288128.patch

lgtm, Andrew do you mind taking a look too?

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8779497 [details] [diff] [review]
1288128.patch

Review of attachment 8779497 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know if its an absolute requirement but do we do certificate pinning on testpilot.firefox.com?

Comment 8

[Andrew Swan [:aswan]]

As discussed over IRC, the patch here won't be of much use by itself until the permissions checking for mozAddonManager (in toolkit/mozapps/extensions/AddonManagerWebAPI.cpp) is also updated.

Comment 9

[Wil Clouser [:clouserw]]

Attachment: Bug 1288178 - add Test Pilot to mozAddonManager allow list

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

Comment 10

[Robert Helmer [:rhelmer]]

Comment on attachment 8779551 [details]
Bug 1288178 - add Test Pilot to mozAddonManager allow list

https://reviewboard.mozilla.org/r/70528/#review67850

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:44
(Diff revision 1)
>      return false;
>    }
>  
>    if (host.Equals("addons.mozilla.org") ||
> -      host.Equals("discovery.addons.mozilla.org")) {
> +      host.Equals("discovery.addons.mozilla.org") ||
> +      host.Equals("tesetpilot.firefox.com")) {

`s/tesetpilot/testpilot/`

Comment 11

[Wil Clouser [:clouserw]]

Comment on attachment 8779551 [details]
Bug 1288178 - add Test Pilot to mozAddonManager allow list

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

Comment 12

[Robert Helmer [:rhelmer]]

Comment on attachment 8779551 [details]
Bug 1288178 - add Test Pilot to mozAddonManager allow list

https://reviewboard.mozilla.org/r/70528/#review67862

Comment 13

[Robert Helmer [:rhelmer]]

Comment on attachment 8779551 [details]
Bug 1288178 - add Test Pilot to mozAddonManager allow list

https://reviewboard.mozilla.org/r/70528/#review67866

Comment 14

[Robert Helmer [:rhelmer]]

Tested this manually and `navigator.mozAddonManager` shows up on https://testpilot.firefox.com for me, going to do a cross-platform xpcshell try run before landing.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2085e633d1f
add Test Pilot to mozAddonManager allow list r=rhelmer

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Wil Clouser [:clouserw]]

Comment on attachment 8779551 [details]
Bug 1288178 - add Test Pilot to mozAddonManager allow list

This is bug 1274332 's cousin.  That bug is about the old way to install add-ons, this bug is about the new way to install add-ons.  Since we just started a new train I'm requesting uplift to aurora and beta.

Approval Request Comment
[Feature/regressing bug #]:
Split out of https://bugzilla.mozilla.org/show_bug.cgi?id=1287125

[User impact if declined]:
We could be losing users who are apprehensive about the add-on install warning.

[Describe test coverage new/current, TreeHerder]:
100% pass on linux/mac/win 64-bit, optimized, xpcshell: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b60b79d37687

[Risks and why]: 
Low risk, adding an entry into existing structures.  We see a large drop off of users who click the add-on install button and then don't install the add-on when prompted/warned.  This allows one-click installs.

[String/UUID change made/needed]: 
none

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8779551 [details]
Bug 1288178 - add Test Pilot to mozAddonManager allow list

Support for testpilot to be installable from Addon Manager. Let's uplift all the way to beta. This should be in beta 4 next week for testing.

Comment 19

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/db7b2b88bfc8

Comment 20

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

https://hg.mozilla.org/releases/mozilla-beta/rev/3485c8d6959b