1751410 - Fix support for private browsing tabs on sitepermission addon with addon id for from xpi signature

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

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

STR:

• install a sitepermission addon with an addon id got from xpi signature (and no addon id assigned from the manifest file properties) for the "midi" or "midi-sysex" site permission
• go to about:addons and allow private browsing access to the site permission addon
• open a tab for the website origin associated to the sitepermission addon and request access to the midi API (await navigator.requestMIDIAccess())
• expected behavior:
  • the request successfully resolves to MIDIAccess instance, no prompt triggered
  • inspecting the website permissions using Services.perms confirms that the private browsing principal has "midi-sysex" permission for the private browsing principal capability stays is set to 2 (DENY_ACTION), while the one for the non private browsing is set to 1 (ALLOW_ACTION).
• actual behavior:
  • prompt opened to ask user if MIDI access should be allowed or blocked
  • inspecting the website permissions using Services.perms should also confirm that both the princpals (private and non private) to have capatibilty set to 1 (ALLOW_ACTION)
    The issue is due to the fact that the SitePermission constructor does not get the addon id from the addonData (e.g. as the Dictionary constructor does here, and also the Extension class constructor does here.

The test case added in Bug 1741750 didn't caught this issue because the test extension xpi ("webmidi_permission.xpi") used does have an explicit addon id in its manifest file properties (set as "[e-mail]" in the applications.gecko.id manifest key) and so it doesn't trigger this scenario.

Comment 1

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

Attachment: Bug 1751410 - SitePermission addon type constructor should retrieve the id from the addonData.id. r?mixedpuppy!

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a7ac5598827
SitePermission addon type constructor should retrieve the id from the addonData.id. r=mixedpuppy

Comment 3

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

Setting qe-verify flag:

This fix should be verified by QA once it got in nightly (and then again after an uplift to beta, if accepted and the patch uplifted to beta 97).

Comment 4

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9a7ac5598827

Comment 5

[Alex Cornestean]

Hello,

Verified the fix on the latest Nightly (98.0a1/20220124214229) under Windows 10 x64 and Ubuntu 16.04 LTS.

After installing the site permission add-on, granting it access to run in Private Windows, opening a Private Window, accessing the origin site and checking the site permission, it can be observed that the site has the permission to access MIDI devices. The site permission pop-up states that access to MIDI devices is “Allowed Temporarily”.

Revoking the add-on access to Private Windows will set the site permission to “Blocked Temporarily”.

Considering these, the fix is confirmed and safe to be uplifted to Beta.

For further details, check the attached screenshot.

Comment 6

[Alex Cornestean]

Attachment: 2022-01-25_09h48_41.png

Comment 7

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

Comment on attachment 9260147 [details]
Bug 1751410 - SitePermission addon type constructor should retrieve the id from the addonData.id. r?mixedpuppy!

Beta/Release Uplift Approval Request

• User impact if declined: The SitePermission addon type will not be able to enable the experimental API gated behind it (only midi/midi-sysex is enabled in Firefox 97 to be gated by a SitePermission addon).
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Same STR used to verify the fix in Nightly.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch is quite small (~1 line change) and restricted to the new SitePermission addon type, no other Firefox internal (included other addon types) can be impacted by the change.
• String changes made/needed:

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Can we add a test for this?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9260147 [details]
Bug 1751410 - SitePermission addon type constructor should retrieve the id from the addonData.id. r?mixedpuppy!

Approved for 97.0b8 in the interest of getting real-world testing. Would be good to know about automated tests still, though.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c090cb86e0ca

Comment 11

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)

Can we add a test for this?

Hi Ryan, sorry for not having noticed the needinfo sooner.

yes, I plan to still add a test case to cover this fix explicitly (the missing test is related to this particular fix, the feature is actually covered by tests landed along with each of the changes) as soon as I can get back to this for a little bit.

I actually took a look into add a new test when I prepared the patch and described a couple of possible approaches in this phabricator comment https://phabricator.services.mozilla.com/D136613#4444293

I'm going to set back the needinfo assigned to me, as a reminder to come back to this to take care of that (either in this issue or in a newly filed separate follow up issue linked to this one).

Comment 12

[Alex Cornestean]

Hello,

Verified the fix on the latest Beta (97.0b8/20220125201015) under Windows 10 x64 and Ubuntu 16.04 LTS.

After installing the site permission add-on, granting it access to run in Private Windows, opening a Private Window, accessing the origin site and checking the site permission, it can be observed that the site has the permission to access MIDI devices. The site permission pop-up states that access to MIDI devices is “Allowed Temporarily”.

Revoking the add-on access to Private Windows will set the site permission to “Blocked Temporarily”.

Considering these, the fix is confirmed.

For further details, check the attached screenshot.

Comment 13

[Alex Cornestean]

Attachment: 2022-01-26_09h53_41.png