1852249 - Permissions of APIs defined outside of toolkit are not listed in the Add-ons Manager

Status: RESOLVED FIXED

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

Description

[John Bieling (:TbSync)]

After moving locales to fluent in bug 1793557, the AddonPermissionsList CustomElement is locked onto the mozilla-central/toolkit locale definitions.

The "default" Localization object is defined here

ExtensionData.formatPermissionStrings is using it as a fallback here

The implementation of AddonPermissionsList is currently not using the localization member of the second parameter of formatPermissionStrings:

• https://searchfox.org/mozilla-central/rev/7fe1954b761abeff36122b4a6ac74619704ee787/toolkit/mozapps/extensions/content/aboutaddons.js#1923-1929
• https://searchfox.org/mozilla-central/rev/7fe1954b761abeff36122b4a6ac74619704ee787/toolkit/mozapps/extensions/content/aboutaddons.js#2008-2011

This causes Thunderbird to print "null" for its own permissions. I assume the android platform will suffer the same issue, if it uses custom android-only permissions.

The proposed patch causes AddonPermissionsList to use this.localization:

    let permissions = Extension.formatPermissionStrings(
      {
        permissions: requiredPerms,
        optionalPermissions: optionalPerms,
      },
      {
        buildOptionalOrigins: manifestV3enabled,
        localization: this.localization,
      }
    );

which allows Thunderbird to override the implementation:

  await customElements.whenDefined("addon-permissions-list");
  AddonPermissionsList.prototype.localization = l10n;

Is there an alternative approach?

Comment 1

[John Bieling (:TbSync)]

Attachment: Bug 1852249 - Allow AddonPermissionsList CustomElement to override localization. r=#extension-reviewers

Comment 2

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

Set release status flags based on info from the regressing bug 1793557

Comment 3

[Rob Wu [:robwu]]

The write-up below elaborates my thought process, and concludes with an alternative approach than proposed here.

The "new" implementation from bug 1793557 offers the ability to define custom permissions and/or permission warnings (for new or existing permission names):

• To add a new permission with a message, its name has to be added to https://searchfox.org/mozilla-central/rev/0afa5b067589195a45ed5e858b51a4ee30c8c0a7/toolkit/components/extensions/ExtensionPermissionMessages.sys.mjs#14-16
  • Already used by Thunderbird at https://searchfox.org/comm-central/rev/933a56297d54b40a9fab147e93eefe226e413f77/mail/modules/ExtensionsUI.jsm#198-219
• To define a new translation that doesn't follow the existing conventions, that mapping has to be added to https://searchfox.org/mozilla-central/rev/0afa5b067589195a45ed5e858b51a4ee30c8c0a7/toolkit/components/extensions/ExtensionPermissionMessages.sys.mjs#48-50
  • The use case for this is when an existing permission receives a new translation, which requires a new l10n ID.

The current implementation assumes all permission strings to be defined by the hardcoded list of files at https://searchfox.org/mozilla-central/rev/0afa5b067589195a45ed5e858b51a4ee30c8c0a7/toolkit/components/extensions/Extension.sys.mjs#79-84
Thunderbird however defines additional strings in messenger/extensionPermissions.ftl, e.g. for permission prompts whose Localization instance override is defined at https://searchfox.org/comm-central/rev/933a56297d54b40a9fab147e93eefe226e413f77/mail/modules/ExtensionsUI.jsm#42

The about:addons code goes through a different code path, and because of that, the default Localization instance without Thunderbird-specific strings is used. The proposed patch patches m-c code to forward a localization instance member to the formatPermissionStrings call (this.localization is undefined on m-c, but intended to be defined in Thunderbird as sketched in the initial report).

The intended way to extend the localizations was through ExtensionPermissionMessages.sys.mjs, with all permission strings defined in toolkit/locales/en-US/toolkit/global/extensionPermissions.ftl. Clearly, this doesn't work sufficiently because the list of ftl files cannot be overridden. I'm not very keen on introducing a seemingly unused instance member.

How about the following:

• Remove the extensionPermissions.ftl reference from Extension.sys.mjs
• Add a new lazy Localization instance in ExtensionPermissionMessages.sys.mjs, which constructs itself based on an exported array.
• From Thunderbird, extend that array with the Thunderbird-specific ftl file, before the first access to any formatPermissionStrings` call.

Comment 4

[John Bieling (:TbSync)]

Attachment: Bug 1852249 - Use a central/exported permission Localization object, to allow builds to modify it. r=#extension-reviewers

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/527e70e2625a
Use a central/exported permission Localization object, to allow builds to modify it. r=extension-reviewers,robwu

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/527e70e2625a

Comment 7

[John Bieling (:TbSync)]

Comment on attachment 9353859 [details]
Bug 1852249 - Use a central/exported permission Localization object, to allow builds to modify it. r=#extension-reviewers

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The fluent migration done for 115 was not complete and other builds like Thunderbird fail to display permission strings. This was not caught in time.
• User impact if declined: Thunderbird ESR115 will not be able to print permission strings.
  https://bug1842490.bmoattachments.org/attachment.cgi?id=9342957
• Fix Landed on Version: 120
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We added tests to verify the fix is working properly.

Comment 8

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

The patch landed in nightly and beta is affected.
:TbSync, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox119 to wontfix.

For more information, please visit BugBot documentation.

Comment 9

[John Bieling (:TbSync)]

Comment on attachment 9353859 [details]
Bug 1852249 - Use a central/exported permission Localization object, to allow builds to modify it. r=#extension-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Other builds (for example Thunderbird) will not be able to print permission strings in the AOM permission tab.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We added tests to verify the fix is working properly.
• String changes made/needed:
• Is Android affected?: Unknown

Comment 10

[Dianna Smith [:diannaS]]

Comment on attachment 9353859 [details]
Bug 1852249 - Use a central/exported permission Localization object, to allow builds to modify it. r=#extension-reviewers

Approved for 119.0b3

Comment 11

[Pulsebot]

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

Comment 12

[(PTO June 19-July 7) Ryan VanderMeulen]

Comment on attachment 9353859 [details]
Bug 1852249 - Use a central/exported permission Localization object, to allow builds to modify it. r=#extension-reviewers

Approved for 115.4esr. Thanks for including a test.

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/342dd9564c17