Restrict mozAddonManager's special privileges to calls from the discovery pane

VERIFIED FIXED in Firefox 68

Status

()

defect
P1
normal
VERIFIED FIXED
3 months ago
2 months ago

People

(Reporter: kmag, Assigned: aswan)

Tracking

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 verified)

Details

Attachments

(2 attachments)

Reporter

Description

3 months ago

mozAddonManager has special powers above those of InstallTrigger to allow one-click installs/enables/disables of add-ons from the discovery pane. Since that functionality isn't meant to be used outside of the discovery pane, we should limit those privileges to usage in the discovery pane, when loaded in about:addons, rather than offering it to AMO in general.

We should also limit it to listed add-ons being installed from AMO, since the discovery pane will never offer unlisted add-ons.

Reporter

Updated

3 months ago
Type: enhancement → defect
Priority: -- → P1
Blocks: 1540173

mozAddonManager has special powers above those of InstallTrigger to allow one-click installs/enables/disables of add-ons from the discovery pane. Since that functionality isn't meant to be used outside of the discovery pane

Currently, AMO and the current web-based discovery pane use mozAddonManager as the main (and only) mechanism to install add-ons. InstallTrigger is not used anymore since August/September 2018.

Assignee

Comment 2

3 months ago

(In reply to William Durand [:willdurand] from comment #1)

Currently, AMO and the current web-based discovery pane use mozAddonManager as the main (and only) mechanism to install add-ons. InstallTrigger is not used anymore since August/September 2018.

Not sure what bearing that has on this bug? InstallTrigger is exposed to all web sites and is publicly documented so we can't remove it. This bug is about changing the behavior of mozAddonManager when it is used outside of the disco pane.

(In reply to Andrew Swan [:aswan] from comment #2)

Not sure what bearing that has on this bug?

Sorry, maybe I misunderstood the original issue but the quote thereafter is not entirely accurate and that's what I meant: AMO and disco pane both have enhanced install buttons with the install/remove/enable/disable features. It's the same button on both apps currently.

mozAddonManager has special powers [...] to allow one-click installs/enables/disables of add-ons from the discovery pane

Also makes

"we should limit those privileges to usage in the discovery pane ... rather than offering it to AMO in general."

a non-starter.

Reporter

Comment 5

3 months ago

(In reply to Andrew Williamson [:eviljeff] from comment #4)

Also makes

"we should limit those privileges to usage in the discovery pane ... rather than offering it to AMO in general."

a non-starter.

It is, in fact, a finisher.

Assignee

Comment 7

2 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a05ac03465cb38ba5a798cb762ccf20a6635f23
Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag

Comment 8

2 months ago
bugherder
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Comment 9

2 months ago
Backout by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/75d0bd84d83f
Backed out changeset 7a05ac03465c for turning bug 1531406 into perma on a CLOSED TREE
Assignee

Comment 12

2 months ago

Nick, can you help me out here? With this patch we now show a confirmation prompt before installing any addons. The existing test of theme installation knows nothing about this prompt so it times out, I could use some help figuring out how to get the test to detect and respond to the prompt.

The prompt is actually launched indirectly through xpcom:
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/mozapps/extensions/AddonManager.jsm#2575-2584
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/mobile/android/components/XPIDialogService.js#16-44

So a terribly hacky solution would just be to override that component for the duration of this test. Can we find something better?

Flags: needinfo?(aswan) → needinfo?(nalexander)
Blocks: 1539591

(In reply to Andrew Swan [:aswan] from comment #12)

Nick, can you help me out here? With this patch we now show a confirmation prompt before installing any addons. The existing test of theme installation knows nothing about this prompt so it times out, I could use some help figuring out how to get the test to detect and respond to the prompt.

The prompt is actually launched indirectly through xpcom:
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/mozapps/extensions/AddonManager.jsm#2575-2584
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/mobile/android/components/XPIDialogService.js#16-44

So a terribly hacky solution would just be to override that component for the duration of this test. Can we find something better?

I see no examples of interacting with Gecko prompts in the existing Robocop tests. Looking at PromptService.java it's clear that we could watch for those native messages and do appropriate things in a text context, but this is pointless: the GeckoView API is different and already tested.

And given that this Robocop test is already mostly expressed in JS, I'd do exactly as you say and use XPCOM wizardry to mock out the prompt around https://searchfox.org/mozilla-central/source/mobile/android/tests/browser/robocop/testThemeInstall.js#46.

For review, flag :petru, :vbaicu, or (as a last resort) me. Good luck!

Flags: needinfo?(nalexander)
Assignee

Comment 14

2 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e2018b550c9e0cb80644b23791e673d152ac3f
Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag

Comment 15

2 months ago
bugherder
Status: REOPENED → RESOLVED
Closed: 2 months ago2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Comment 16

2 months ago

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Flags: needinfo?(aswan)
Assignee

Comment 17

2 months ago

Manual testing can be pretty simple:

  1. Install a theme from the discovery pane ("Get Extensions" in about:addons), no prompt should be shown before installation
  2. Install a theme from addons.mozilla.org, a prompt should be shown before installation
Flags: needinfo?(aswan)

Comment 18

2 months ago
Posted image Bug1539598.gif

This issue is verified as fixed on Firefox 68.0a1 (20190421212635) under Win 7 64-bit and Mac OS X 10.14.1.

Please see the attached video.

Updated

2 months ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.