Closed Bug 1751410 Opened 2 years ago Closed 2 years ago

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

Categories

(WebExtensions :: General, defect, P1)

defect
Points:
2

Tracking

(firefox97 verified, firefox98 verified)

VERIFIED FIXED
98 Branch
Tracking Status
firefox97 --- verified
firefox98 --- verified

People

(Reporter: rpl, Assigned: rpl, NeedInfo)

References

Details

(Whiteboard: [addons-jira])

Attachments

(3 files)

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 "webmidi@test.mozilla.org" in the applications.gecko.id manifest key) and so it doesn't trigger this scenario.

Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/9a7ac5598827
SitePermission addon type constructor should retrieve the id from the addonData.id. r=mixedpuppy

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).

Flags: qe-verify+
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch

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.

Status: RESOLVED → VERIFIED
Attached image 2022-01-25_09h48_41.png

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:
Attachment #9260147 - Flags: approval-mozilla-beta?

Can we add a test for this?

Flags: needinfo?(lgreco)

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.

Attachment #9260147 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

(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).

Flags: needinfo?(lgreco)
Flags: needinfo?(lgreco)

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.

Attached image 2022-01-26_09h53_41.png
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: