Legacy extension includes an unsigned extension

RESOLVED FIXED in Firefox 56

Status

()

P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: andy+bugzilla, Assigned: aswan)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: triaged)

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Created attachment 8876228 [details]
Screenshot 2017-06-09 10.21.10.png

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

Comment 1

a year ago
This will only occur if you have at least one legacy extension.
(Reporter)

Updated

a year ago
Duplicate of this bug: 1371749

Updated

a year ago
Assignee: nobody → aswan
Priority: -- → P2
Whiteboard: triaged
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8877318 - Flags: review?(rhelmer)

Comment 4

a year ago
mozreview-review
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+
(Assignee)

Comment 5

a year ago
(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)
(Assignee)

Comment 7

a year ago
(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.

Comment 8

a year ago
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
https://hg.mozilla.org/mozilla-central/rev/6166e9796a59
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.