1778959 - Prototype an AOM SitePermsAddonProvider to support synthetic Site Permissions addon type install flows

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, task, P3)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

This bug goal is to prototype a new AOM provider to support a synthetic Site Permission addon type, which should:

• derive the list of synthetic SitePermsAddon instances from the list of site origins with any addon gated permission granted;
• provide a synthetic install flow (which should provide the same kind of install flow that the previous signed xpi version of the "sitepermission" addon type was providing) triggered when a gated addon permission is being requested.

The goal of the prototype is to help confirming that an AOM provider would be a viable, clean and simple enough approach, and if that is confirmed and if we will agree to proceed with that approach to then help informing and guiding the actual implementation work.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1778959 - SitePermsAddonProvider basic boilerplate skeleton. r=rpl!,willdurand!.

This patch includes a basic (but still working) version of a new SitePermsAddonProvider.

NOTE:

• In this basic version of the provider, the list of synthetic addons is recomputed from
  the list of the granted permission on each call to getAddonByID / getAddonsByType
  that matches the parameters expected by the provider.
• These two provider methods (getAddonBYID / getAddonsByTypes) are in practice only called
  when the user navigates to about:addons, but it may be reasonable to iterate on this
  initial prototype by introducing a cache file where we can list all origins with a gated
  addon permission and keep that cache file updated through an "perm-changed" notification
  observer and a new scan of all permission on each provider startup.

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1778959 - Add method to AddonManager for installing synthetic SitePermsAddon. r=rpl!,willdurand!.

This patch includes:

• a new AddonManager method to trigger a new SitePermsAddon install flow
• a few more changes to the SitePermsAddonProvider to support the synthetic addon install flow

Depends on D151476

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1778959 - Tweaks to PermissionUI to delegate gated site permissions request flows to synthetic SitePermsAddon install flow. r=#permissions-reviewers!

This patch includes a new PermissionUI.SitePermsAddonInstallRequestPrototype helper, it inherits from PermissionPromptForRequestPrototype
and override the prompt method to trigger the synthetic SitePermsAddon install flow when a addon gated site permissions is requested
(currently only "midi/midi-sysex").

The new SitePermsAddonInstallRequestPrototype helper is then set as the proto for the MIDIPermissionPrompt.prototype, which
makes sure that every time the MIDI site permissions are being requested, the user is prompted to install a synthetic addon
as part of the site permission request.

NOTE:

• the rest of the MIDIPermissionPrompt is currently unused in this version of this exploratory patch, a finalized version
  of the patch should also consider if there is any scenario that would require the regular permission prompt for the
  gated site permission requests

Depends on D151477

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

As a side note:

• Along with the 3 patches attached to the bug, the following check in MIDIPermissionsRequest::Run needs to be removed (or commented out) to make sure requestMIDIAccess will trigger the install flow (by reaching the call to “nsContentPermissionUtils::AskPermission” that follows it):

NS_IMETHODIMP
MIDIPermissionRequest::Run() {
  ...
   // If the add-on is not installed, auto-deny (except for localhost).
  if (!nsContentUtils::HasSitePerm(mPrincipal, kPermName) &&
      !BasePrincipal::Cast(mPrincipal)->IsLoopbackHost()) {
    Cancel();
    return NS_OK;
  }
  ...
}

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

I pulled the patches today and tried to see the various parts of the work, using navigator.requestMIDIAccess() to trigger the prompts.
If the installation of the addon doesn't go through, the promise rejects with the following error: https://searchfox.org/mozilla-central/rev/f655bdf6b4bf01b42609750ab94fc37635397260/dom/midi/MIDIPermissionRequest.cpp#62-65

I guess we'll have to tweak this a bit a it probably refers to a previous implementation

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)

I pulled the patches today and tried to see the various parts of the work, using navigator.requestMIDIAccess() to trigger the prompts.
If the installation of the addon doesn't go through, the promise rejects with the following error: https://searchfox.org/mozilla-central/rev/f655bdf6b4bf01b42609750ab94fc37635397260/dom/midi/MIDIPermissionRequest.cpp#62-65

I guess we'll have to tweak this a bit a it probably refers to a previous implementation

yep, I totally agree, that error message would need to be tweaked accordingly to how this will be working once there isn't any website opt-in.

Honestly I think that it also be reasonable to make that error to just states that the user rejected the permission request, the developer are not going to be opting in by installing an addon in this version of the sitepermission addon type and so:

• from a website perspective, that rejection means that the user didn't let the install flow to complete (and basically not granted the permission request)
• from a user perspective, that error wouldn't be directly visible, and the users wouldn't definitely need to look to our extension developers docs.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1778959 - Honor dom.sitepermsaddon-provider.enabled for Midi permission requests. r=timhuang,willdurand.

When the pref is true, we want to go through the new SitePermissionAddon
install flow.

Depends on D151478

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1778959 - Don't expose requestMIDIAccess on file scheme. r=bholley.

Depends on D156192

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1778959 - Expose nsIPrincipal.isLoopbackHost. r=bholley.

This will be used in SitePermsAddonInstallRequestPrototype to discriminate permission
requests coming from localhost, where we want to fallback to regular permission prompt.

Depends on D151477

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a68797d8bbc
SitePermsAddonProvider basic boilerplate skeleton. r=rpl,willdurand.
https://hg.mozilla.org/integration/autoland/rev/58083286fe3d
Add method to AddonManager for installing synthetic SitePermsAddon. r=rpl,willdurand.
https://hg.mozilla.org/integration/autoland/rev/2f1a9eb03661
Expose nsIPrincipal.isLoopbackHost. r=bholley.
https://hg.mozilla.org/integration/autoland/rev/3913eb81b668
Tweaks to PermissionUI to delegate gated site permissions request flows to synthetic SitePermsAddon install flow. r=permissions-reviewers,pbz
https://hg.mozilla.org/integration/autoland/rev/21a65ff1007c
Honor dom.sitepermsaddon-provider.enabled for Midi permission requests. r=willdurand,timhuang.
https://hg.mozilla.org/integration/autoland/rev/1ef96ac13291
Don't expose requestMIDIAccess on file scheme. r=bholley.

Comment 11

[Sandor Molnar]

https://hg.mozilla.org/mozilla-central/rev/1a68797d8bbc
https://hg.mozilla.org/mozilla-central/rev/58083286fe3d
https://hg.mozilla.org/mozilla-central/rev/2f1a9eb03661
https://hg.mozilla.org/mozilla-central/rev/3913eb81b668
https://hg.mozilla.org/mozilla-central/rev/21a65ff1007c
https://hg.mozilla.org/mozilla-central/rev/1ef96ac13291

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1778959 - Disable failing test_sitePermsAddonProvider.js in Thunderbird. r=nchevobbe,rpl,willdurand

This feature is explicitly disabled in Thunderbird, in extensions.manifest, so the test doesn't work there.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3ff2257134f
Disable failing test_sitePermsAddonProvider.js in Thunderbird. r=willdurand

Comment 15

[Atila Butkovits]

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