Closed Bug 1288178 Opened 4 years ago Closed 4 years ago

Allow testpilot to use mozAddonManager

Categories

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

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: andy+bugzilla, Assigned: clouserw)

Details

(Whiteboard: triaged)

Attachments

(1 file, 1 obsolete file)

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.
Priority: -- → P2
Whiteboard: triaged
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?
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.
Flags: needinfo?(amckay)
Attached patch 1288128.patch (obsolete) — Splinter Review
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!
Attachment #8779497 - Flags: review?(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.
Assignee: nobody → wclouser
Flags: needinfo?(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
Flags: needinfo?(aswan)
Comment on attachment 8779497 [details] [diff] [review]
1288128.patch

lgtm, Andrew do you mind taking a look too?
Attachment #8779497 - Flags: review?(rhelmer)
Attachment #8779497 - Flags: review?(aswan)
Attachment #8779497 - Flags: review+
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?
Attachment #8779497 - Flags: review?(aswan) → review+
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 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/`
Attachment #8779551 - Flags: review?(rhelmer) → review-
Comment on attachment 8779551 [details]
Bug 1288178 - add Test Pilot to mozAddonManager allow list

https://reviewboard.mozilla.org/r/70528/#review67862
Attachment #8779551 - Flags: review?(rhelmer) → review-
Comment on attachment 8779551 [details]
Bug 1288178 - add Test Pilot to mozAddonManager allow list

https://reviewboard.mozilla.org/r/70528/#review67866
Attachment #8779551 - Flags: review- → review+
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.
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2085e633d1f
add Test Pilot to mozAddonManager allow list r=rhelmer
https://hg.mozilla.org/mozilla-central/rev/b2085e633d1f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Attachment #8779497 - Attachment is obsolete: true
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
Attachment #8779551 - Flags: approval-mozilla-beta?
Attachment #8779551 - Flags: approval-mozilla-aurora?
Flags: needinfo?(amckay)
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.
Attachment #8779551 - Flags: approval-mozilla-beta?
Attachment #8779551 - Flags: approval-mozilla-beta+
Attachment #8779551 - Flags: approval-mozilla-aurora?
Attachment #8779551 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.