Closed Bug 1371752 Opened 8 years ago Closed 8 years ago

Legacy extension includes an unsigned extension

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: andy+bugzilla, Assigned: aswan)

References

Details

(Whiteboard: triaged)

Attachments

(2 files)

On Nightly 1. turn off extension signing requirement 2. turn on legacy add-on restrictions 3. install an unsigned add-on. The add-on my add-ons list, it also appears in the legacy extensions list. Because that page says at the top "... we deactivated them" that's confusing. Perhaps we can leave add-ons off that page if the extension signing requirement is turned off. This will never happen in release, so prioritize accordingly.
This will only occur if you have at least one legacy extension.
Assignee: nobody → aswan
Priority: -- → P2
Whiteboard: triaged
Attachment #8877318 - Flags: review?(rhelmer)
Comment on attachment 8877318 [details] Bug 1371752 Don't show unsigned extensions in legacy pane if signatures are disabled https://reviewboard.mozilla.org/r/148656/#review153572 I'm not sure I really agree with the premise of this bug... a legacy extension is a legacy extension, regardless of whether signing is enabled on the build, right? I don't feel strongly enough to r- for that reason alone though. ::: toolkit/mozapps/extensions/content/extensions.js:320 (Diff revision 1) > // Add-ons without an "isCorrectlySigned" property are correctly signed as > // they aren't the correct type for signing. > return aAddon.isCorrectlySigned !== false; > } > > +function isDisabledUnsigned(addon) { This function name is confusing to me, I read it as "is this add-on both disabled and unsigned". What do you think? Would something like `isSigningDisabled(addon)` be clearer?
Attachment #8877318 - Flags: review?(rhelmer) → review+
(In reply to Robert Helmer [:rhelmer] from comment #4) > I'm not sure I really agree with the premise of this bug... a legacy > extension is a legacy extension, regardless of whether signing is enabled on > the build, right? I don't feel strongly enough to r- for that reason alone > though. Well we still put a warning on individual unsigned extensions. The point here is that the UX is meant to separate extensions that you are using (or can use but have manually disabled) from those that you just can't use at all. With usable extensions in the Extensions pane and unusable ones in the "legacy" pane. In the specific case of Nightly with the signatures pref flipped, unsigned webextensions are usable so they shouldn't appear in the "unusable" list. > > +function isDisabledUnsigned(addon) { > > This function name is confusing to me, I read it as "is this add-on both > disabled and unsigned". > > What do you think? Would something like `isSigningDisabled(addon)` be > clearer? Oh, it was mean to be a more concise version of "this is disabled because it is unsigned". I think isSigningDisabled is confusing, it sounds to me like it is about the global setting, not about an individual extension.
(In reply to Andrew Swan [:aswan] from comment #5) > (In reply to Robert Helmer [:rhelmer] from comment #4) > > > +function isDisabledUnsigned(addon) { > > > > This function name is confusing to me, I read it as "is this add-on both > > disabled and unsigned". > > > > What do you think? Would something like `isSigningDisabled(addon)` be > > clearer? > > Oh, it was mean to be a more concise version of "this is disabled because it > is unsigned". I think isSigningDisabled is confusing, it sounds to me like > it is about the global setting, not about an individual extension. Fair enough... since it takes addon as an arg I was hoping it'd read like "is signing disabled for this add-on" but that is confusing too. The core issue I have is that "disabled" makes me think of the add-on state (user/app disabled etc)
(In reply to Robert Helmer [:rhelmer] from comment #6) > The core issue I have is that "disabled" makes me think of the add-on state > (user/app disabled etc) Well it is certainly meant to mirror that. If this function returns true, then the addon.appDisabled should also be true. Of course, the addon could be appDisabled for some other reason as well so the reverse isn't true.
Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6166e9796a59 Don't show unsigned extensions in legacy pane if signatures are disabled r=rhelmer
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: