Closed Bug 1170841 Opened 5 years ago Closed 4 years ago

Show warning in add-on manager for add-ons that aren't properly signed

Categories

(Firefox for Android :: Add-on Manager, defect)

35 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 41

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(5 files)

Android version of bug 1149702.

antlam, do you have any ideas about a (super simple) indicator we could use on add-ons that aren't properly signed?
Flags: needinfo?(alam)
Attached image prev_aboutaddons.png
Indeed! we could use the same icon (as the dialog) and place it to the right hand side (not unlike the styling for the Home panels layouts I've been working on)
Flags: needinfo?(alam)
Attached image icon_greycaution.svg
Here's the icon in .SVG
Assignee: nobody → margaret.leibovic
I think we should also include a message in the details page that explains why this add-on is disabled, since it's not clear what the warning icon there means.

On desktop, there's also a warning even if we don't force these add-ons to be disabled. We should look at the destkop about:addons UI for inspiration here.
Flags: needinfo?(alam)
Attached image prev_aboutaddons2.png
Agree, I had this put aside for what happens when the user presses into an add-on. The copy is mostly taken from what's currently in desktop preferences. I made it more concise cause I think it's a bit dated by now.
Flags: needinfo?(alam)
^ Also, I noticed something weird going on with the buttons (uninstall and install) in that page. That's not in the design, I just moved the image around for the mock. 

We should just clean up the strokes and corners to be consistent.
Flags: needinfo?(margaret.leibovic)
(In reply to Anthony Lam (:antlam) from comment #5)
> ^ Also, I noticed something weird going on with the buttons (uninstall and
> install) in that page. That's not in the design, I just moved the image
> around for the mock. 
> 
> We should just clean up the strokes and corners to be consistent.

Can you elaborate more on what's going wrong with the buttons? We can file a separate bug for this, but I'm happy to clean things up while I'm in here.

I think this design looks good, I'll get to working on this. But just FYI, if add-on signing is required, the "Disable" button in this case would actually be "Enable", and it would be disabled, since we wouldn't allow the user to change that state.
Flags: needinfo?(margaret.leibovic)
Done! Filed bug 1175197 for the clean up.
Blocks: 1175741
Bug 1170841 - Show warning in add-on manager for add-ons that aren't properly signed. r=liuche
Attachment #8623985 - Flags: review?(liuche)
Attached image screenshot
Comment on attachment 8623985 [details]
MozReview Request: Bug 1170841 - Show warning in add-on manager for add-ons that aren't properly signed. r=liuche

https://reviewboard.mozilla.org/r/11639/#review10409

::: mobile/android/chrome/content/aboutAddons.js:237
(Diff revision 1)
> +    item.setAttribute("isUnsigned", aAddon.signedState <= AddonManager.SIGNEDSTATE_MISSING);

So did this used to display a "corrupt file" error for the addon (instead of "missing)?

It seems like this possible null value should be checked explicitly in the AddonInstall code when checking for SIGNEDSTATE_MISSING, instead of being implicitly checked by a null <= SIGNEDSTATE_MISSING and then falling through to an ERROR_CORRUPT_FILE - the caller shouldn't need to explicitly set it.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5472

However, this seems much less low risk because it only touches code here, so I'm also okay with that.
Attachment #8623985 - Flags: review?(liuche) → review+
(In reply to Chenxia Liu [:liuche] from comment #10)
> Comment on attachment 8623985 [details]
> MozReview Request: Bug 1170841 - Show warning in add-on manager for add-ons
> that aren't properly signed. r=liuche
> 
> https://reviewboard.mozilla.org/r/11639/#review10409
> 
> ::: mobile/android/chrome/content/aboutAddons.js:237
> (Diff revision 1)
> > +    item.setAttribute("isUnsigned", aAddon.signedState <= AddonManager.SIGNEDSTATE_MISSING);
> 
> So did this used to display a "corrupt file" error for the addon (instead of
> "missing)?

I don't think I understand the question... do you mean did we display some message about the add-on if it had another problem? Our about:addons logic is pretty bare-bones, so I don't think we displayed any message about add-on problems.

> It seems like this possible null value should be checked explicitly in the
> AddonInstall code when checking for SIGNEDSTATE_MISSING, instead of being
> implicitly checked by a null <= SIGNEDSTATE_MISSING and then falling through
> to an ERROR_CORRUPT_FILE - the caller shouldn't need to explicitly set it.
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#5472

I basically just copied this logic from the desktop implementation, and I believe the idea here is to display a generic error to the user any time there's a problem with signing, so that's why we don't have more nuanced messages for the different types of signing error states.

We can check with Mossop to see if this is something that we would ever want to change.
Flags: needinfo?(dtownsend)
(In reply to :Margaret Leibovic from comment #12)
> (In reply to Chenxia Liu [:liuche] from comment #10)
> > Comment on attachment 8623985 [details]
> > MozReview Request: Bug 1170841 - Show warning in add-on manager for add-ons
> > that aren't properly signed. r=liuche
> > 
> > https://reviewboard.mozilla.org/r/11639/#review10409
> > 
> > ::: mobile/android/chrome/content/aboutAddons.js:237
> > (Diff revision 1)
> > > +    item.setAttribute("isUnsigned", aAddon.signedState <= AddonManager.SIGNEDSTATE_MISSING);
> > 
> > So did this used to display a "corrupt file" error for the addon (instead of
> > "missing)?
> 
> I don't think I understand the question... do you mean did we display some
> message about the add-on if it had another problem? Our about:addons logic
> is pretty bare-bones, so I don't think we displayed any message about add-on
> problems.
> 
> > It seems like this possible null value should be checked explicitly in the
> > AddonInstall code when checking for SIGNEDSTATE_MISSING, instead of being
> > implicitly checked by a null <= SIGNEDSTATE_MISSING and then falling through
> > to an ERROR_CORRUPT_FILE - the caller shouldn't need to explicitly set it.
> > 
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> > internal/XPIProvider.jsm#5472
> 
> I basically just copied this logic from the desktop implementation, and I
> believe the idea here is to display a generic error to the user any time
> there's a problem with signing, so that's why we don't have more nuanced
> messages for the different types of signing error states.
> 
> We can check with Mossop to see if this is something that we would ever want
> to change.

Yeah we decided that the user didn't need to know what the specific problem was, just that the add-ons wasn't signed properly. They can't really act on anything more specific than that.

Note that if an add-on doesn't need to be signed its signedState is undefined, not null and (undefined <= SIGNEDSTATE_MISSING) === false so things should work out fine.
Flags: needinfo?(dtownsend)
Looks like this missed getting closed...

http://hg.mozilla.org/mozilla-central/rev/e796ac2a6f7a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Verified as fixed on Firefox 41 Beta 1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.