Closed Bug 989806 Opened 6 years ago Closed 6 years ago

Use reviewers certificate when installing apps from the Marketplace reviewer pages

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set

Tracking

(firefox31 fixed, firefox32 fixed)

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(3 files, 6 obsolete files)

We need to use the reviewers certificate when installing apps from the Marketplace reviewers page, so that reviewers can install apps and review them.

There's a discussion in bug 889744.
Attached patch Patch (obsolete) — Splinter Review
I've used some real apps downloaded from the marketplace, it would be better if we had some example apps.
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #8399174 - Flags: review?(fabrice)
Fabrice, do you know who could provide sample apps signed by Marketplace?
Wil, can we get a sample "Hello World" app signed by the marketplace?
Flags: needinfo?(clouserw)
If possible, I'd need:
 - a sample privileged app signed by Marketplace;
 - a sample packaged app signed by Marketplace;
 - a sample packaged app signed with the Marketplace reviewers certificate.

(The results are the same with either privileged or packaged apps, but I'd like to test them both for completeness)
privileged app signed by marketplace:
   https://marketplace.firefox.com/downloads/file/210676/kitchensink-0.2.2.zip

sample packaged app signed by marketplace:
   https://marketplace.firefox.com/downloads/file/227650/flashlight-linterna-2.0.zip

sample packaged app signed w/ reviewers cert:
   bwalker recommends we don't link to this file in a public bug.  I don't see you on IRC so I'll email the file to you.
Flags: needinfo?(clouserw)
Well, it's gonna end up in a public repo..,
Flags: needinfo?(bwalker)
(In reply to Fabrice Desré [:fabrice] from comment #6)
> Well, it's gonna end up in a public repo..,

The idea is that we're going to use this in an automated test, right? I guess putting that app here is OK; it's not as though we're revealing the private key we used to sign the app.
Flags: needinfo?(bwalker)
Attached file 1434790.zip
sample app signed by reviewer cert
Attached patch Patch (obsolete) — Splinter Review
Updated with the new sample apps (thank you!).
Attachment #8399174 - Attachment is obsolete: true
Attachment #8399174 - Flags: review?(fabrice)
Attachment #8410485 - Flags: review?(fabrice)
Attachment #8410485 - Flags: review?(fabrice) → review+
Attached patch Patch (obsolete) — Splinter Review
Updated commit message.
Attachment #8410485 - Attachment is obsolete: true
Attachment #8411739 - Flags: review+
Keywords: checkin-needed
Blocks: 889744
This needs major rebasing on inbound at least.
Keywords: checkin-needed
I've written a wrong bug number in the patch (as it was at the beginning a patch for bug 889744), so it's already landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/b589de6c1084
I pushed a version that calls SimpleTest.requestCompleteLog() to tryserver to see if it provides any useful context for the nsIPromptFactory::getPrompt failure that precedes the timeout:

  https://tbpl.mozilla.org/?tree=Try&rev=c01b343bef8c
The failure looks similar to bug 948948.
It is failing right after the iframe is added to the DOM ("domParent.appendChild(ifr);" in the installApp function).
Ok, I've fixed the error by setting the "mozbrowser" property of the iframe to "true" and by adding the "browser" permission at the beginning of the test.
It is still failing on B2G Desktop, now because of another reason: the error page "The connection is untrusted" is shown.
Isn't this supposed to be solved by the new certs in build/pgo?
Blocks: 972201
Attached patch Patch (obsolete) — Splinter Review
Here's the last version of the patch.
Fails on b2g with "mozbrowsererror: certerror".

I've found two tests that load an https page in an iframe: dom/browser-element/mochitest/browserElement_SecurityChange.js and dom/browser-element/mochitest/browserElement_ErrorSecurity.js.

The former is disabled on Android (bug 766586) and on B2G (no bug is referenced and there isn't a comment explaining why).

The latter is disabled on b2g, there's a comment that says: "Disable due to certificate issue (no bug that I'm aware of)".

I've just found bug 907770, as it turns out until a few days ago HTTPS hosts in b2g and Android mochitests weren't supported. I'm still experiencing the cert issue though.
Attachment #8411739 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Could you review the mochitest.ini changes?
I think we can land this disabled on b2g until we fix the certificate issue.

Try run is green: https://tbpl.mozilla.org/?tree=Try&rev=546171fce13e
Attachment #8413189 - Attachment is obsolete: true
Attachment #8413342 - Flags: review?(ryanvm)
Comment on attachment 8413342 [details] [diff] [review]
Patch

Assuming your intent was disable the test on all B2G builds (desktop and emulator), this looks correct to me.
Attachment #8413342 - Flags: review?(ryanvm) → feedback+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21)
> Comment on attachment 8413342 [details] [diff] [review]
> Patch
> 
> Assuming your intent was disable the test on all B2G builds (desktop and
> emulator), this looks correct to me.

Yes, it has to be disabled on all b2g builds (as the dom/browser-element tests that involve HTTPS hosts).
Attached patch Patch (obsolete) — Splinter Review
Rebased (there was a conflict in mochitest.ini).
Attachment #8413342 - Attachment is obsolete: true
Attachment #8413429 - Flags: review+
Keywords: checkin-needed
Any reason the test wasn't added to the manifest alphabetically?
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> Any reason the test wasn't added to the manifest alphabetically?

No reason, I didn't know there was a ordering rule.
Just good practice :)
It's failing because of the certificate issue on Android too... But it is an intermittent failure (and it looks like 2.2 specific).
I wonder if this is a problem with the mochitest suite or a problem with the mozbrowser iframe.
Attached patch PatchSplinter Review
Looks like tests using mozbrowser may be unreliable on Android (see bug 855543 comment 21).
Let's land with the test disabled on both b2g and Android.
Attachment #8413429 - Attachment is obsolete: true
Attachment #8413853 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8234dac0ea59
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Can we have this patch uplifted to Aurora?  Reviewing app submissions on Nightly is a little risky (we might reject for a transient platform bug) and we'd like to get a working solution in place as soon as possible.
Attached patch Patch for AuroraSplinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 896620
User impact if declined: Reviewers will not be able to install apps from Marketplace reviewers page (and so will not be able to review apps).
Testing completed (on m-c, etc.): This has been on m-c for a week or so, it landed right after the mozilla-central -> mozilla-aurora transition.
Risk to taking this patch (and alternatives if risky): The actual code change is small, the patch looks big because it contains test apps and new mochitest certificates for a new HTTPS host.
String or IDL/UUID changes made by this patch: None.
Attachment #8418940 - Flags: approval-mozilla-aurora?
Attachment #8418940 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.