Closed Bug 762964 Opened 11 years ago Closed 11 years ago

Add a test page for 'submit unencrypted data' to litmus-data repository

Categories

(Mozilla QA Graveyard :: Infrastructure, defect)

Product:
Mozilla QA Graveyard
Component:
Infrastructure
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: AlexLakatos, Assigned: AlexLakatos)

References

Details

Attachments

(2 files, 9 obsolete files)

patch v8.1
1.75 KB, patch
davehunt
: review+
Details | Diff | Splinter Review
follow-up patch v2.0
1.86 KB, patch
whimboo
: review+
Details | Diff | Splinter Review 
Need a test page to submit unencripted data. Needed for testSecurity/testSubmitUnencryptedInfoWarning.js
Assignee: nobody → alex.lakatos
Blocks: 725486
Status: NEW → ASSIGNED
Attached patch patch v1.0 (obsolete) — — Splinter Review 
Replicated the same functionality as on mozilla.org. Can we access localhost with https? If not, there is no sense in porting this to mozmill-tests/data
Attachment #631439 - Flags: review?(hskupin)
Summary: Add a test page to submit unencripted data to litmus-data repository → Add a test page for 'submit unencrypted data' to litmus-data repository
Comment on attachment 631439 [details] [diff] [review]
patch v1.0

>+  <form action="http://www.google.com/cse" title="Search Mozilla sites">

Please do not escape from the current domain. Just replace the protocol from the URL of the page for the form action URL.

>+    <input type="hidden" name="cx" value="002443141534113389537:ysdmevkkknw">
>+    <input type="hidden" name="cof" value="FORID:0">

Those are not necessary.

>+    <input id="q" type="search" name="q" placeholder="Search">

Minimize that please.
Attachment #631439 - Flags: review?(hskupin) → review-
Attached patch patch v2.0 (obsolete) — — Splinter Review 
minimized and used js so not to escape the domain
Attachment #631439 - Attachment is obsolete: true
Attachment #644964 - Flags: review?(hskupin)
Comment on attachment 644964 [details] [diff] [review]
patch v2.0

>+  <title>Unencripted Search</title>

nit: unencrypted.

>+  <script>
>+    function init() {
>+      document.form.action = "http://" + location.host;

So if only a single form exists in the page we have the 'form' property on the document? I wonder if we should make it better us 'document.forms[0].action' instead. To be more save we should give the form an id and get this element in init().

>+<body onload="init();">
>+  <form action="" name="form">
>+    <input id="q">
>+  </form>
>+</body>

Please add a paragraph above the form which adds some content for what this test file is needed and that it initially has to be opened via HTTPS.
Attachment #644964 - Flags: review?(hskupin) → review-
Attached patch patch v3.0 (obsolete) — — Splinter Review 
(In reply to Henrik Skupin (:whimboo) from comment #4)
> So if only a single form exists in the page we have the 'form' property on
> the document?
No. There is no 'form' property on the document
>I wonder if we should make it better us
> 'document.forms[0].action' instead. To be more save we should give the form
> an id and get this element in init().
I'm using a name already. That's why I can use document.form. 'form' is the name I gave to the form tag.

Added explanatory paragraph.
Attachment #644964 - Attachment is obsolete: true
Attachment #645227 - Flags: review?(hskupin)
Comment on attachment 645227 [details] [diff] [review]
patch v3.0

>+  <p>Testcase used for Mozmill <a href="http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testSecurity/testSubmitUnencryptedInfoWarning.js">testSecurity/testSubmitUnencryptedInfoWarning.js</a></p>

I don't think we should put in a link here. Remove that paragraph.

>+  <p>Needs to be opened using the https protocol</p>

You should say something like: "To ensure this testcase is working as expected, it has to be loaded via an SSL connection."

>+  <form action="" name="form">

Don't use name but id. Name shouldn't longer be used with HTML5.
Attachment #645227 - Flags: review?(hskupin) → review-
Attached patch patch v4.0 (obsolete) — — Splinter Review 
Using an id, removed first paragraph, changed the text in the second.
Attachment #645227 - Attachment is obsolete: true
Attachment #645304 - Flags: review?(hskupin)
Comment on attachment 645304 [details] [diff] [review]
patch v4.0

>+++ b/firefox/search/unencryptedsearch.html

Sorry that I missed that but this testcase really has to be located under security.

>+  <form action="" id="form">
>+    <input id="q">
>+  </form>

Please add a submit button.

Otherwise it looks fine.
Attachment #645304 - Flags: review?(hskupin) → review-
Attached patch patch v5.0 (obsolete) — — Splinter Review 
moved and added a submit button
Attachment #645304 - Attachment is obsolete: true
Attachment #645849 - Flags: review?(hskupin)
Comment on attachment 645849 [details] [diff] [review]
patch v5.0

Partly tested locally and have seen two things.

>+      document.getElementById("form").action = "http://" + location.host;

Instead of specifying the host only we should make use of the same web page but simply replace https with http. Reason see below.

>+  <p>To ensure this testcase is working as expected, it has to be loaded via an SSL connection.</p>
>+  <form action="" id="form">
>+    <input id="q">

The tests we have which need this testcase are testing the target page for the search field and if the value entered in the first page is shown. So we should fill-in any specified value given by the q parameter to that file.

If we don't do this, there is no way to check that the search was successful even that we use http now.
Attachment #645849 - Flags: review?(hskupin) → review-
Attached patch patch v6.0 (obsolete) — — Splinter Review 
Form points to the same page. Search term is now displayed on the page.
To test out the functionality of the search, but not the functionality of the unencrypted search, I've uploaded it on my own website:
http://greensqr.com/misc/mozilla/unencryptedsearch.html
Attachment #645849 - Attachment is obsolete: true
Attachment #646087 - Flags: review?(hskupin)
Comment on attachment 646087 [details] [diff] [review]
patch v6.0

>+    function init() {
>+      document.getElementById("form").action = "http://" + location.host + location.pathname;

Can you simply replace 'https' with 'http'? That would also keep the port intact.

>+        document.getElementById("search-term").textContent = searchTerm;
[..]
>+  <p>The search term was <span id="search-term"></span></p>
>+  <form id="form" method="get" action="">
>+    <input name="q" id="q">
>+    <input type="submit" id="submit">

I think we should fill-in the search term into the input field. That gives us nearly a 1:1 experience with the Google search page and let us and users who check this file manually better get the term.
Attachment #646087 - Flags: review?(hskupin) → review-
Attached patch patch v7.0 (obsolete) — — Splinter Review 
Used css after all, seemed to make the JS part a little lighter.
Attachment #646087 - Attachment is obsolete: true
Attachment #646134 - Flags: review?(hskupin)
Comment on attachment 646134 [details] [diff] [review]
patch v7.0

>Bug 762964 - Add a test page to submit unencripted data to litmus-data repository. r=hskupin

Typo: unencripted/unencrypted

>diff --git a/firefox/security/unencryptedsearch.html b/firefox/security/unencryptedsearch.html

Shouldn't this be in data/security?

Otherwise, this looks good to me.

Note that while Henrik is on PTO please assigned reviews to me after reviewing them with your local team.
Attachment #646134 - Flags: review?(hskupin) → feedback+
Comment on attachment 646134 [details] [diff] [review]
patch v7.0

Sorry, incorrect flag set previously.
Attachment #646134 - Flags: feedback+ → review-
Attached patch patch v8.0 (obsolete) — — Splinter Review 
> >diff --git a/firefox/security/unencryptedsearch.html b/firefox/security/unencryptedsearch.html
> 
> Shouldn't this be in data/security?
No,there is no data folder. I'm following the directory structure of the litmus-data repo.
Attachment #646134 - Attachment is obsolete: true
Attachment #646618 - Flags: review?(dave.hunt)
Attached patch patch v8.1 — — Splinter Review 
Bug summary changed in the meantime, and seems we all missed that in the commit message. Updated it now.
Attachment #646618 - Attachment is obsolete: true
Attachment #646618 - Flags: review?(dave.hunt)
Attachment #646621 - Flags: review?(dave.hunt)
Comment on attachment 646621 [details] [diff] [review]
patch v8.1

That makes complete sense! Landed as:
https://hg.mozilla.org/qa/litmus-data/rev/47c7f4450fc9
Attachment #646621 - Flags: review?(dave.hunt) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 646621 [details] [diff] [review]
patch v8.1

>+        var newParagraph = document.createElement("p");
>+        var newSpan = document.createElement("span");
>+
>+        newParagraph.id = "search-results";
>+        newParagraph.textContent = "Search term was ";
>+        newSpan.id = "search-term";
>+        newSpan.textContent = searchTerm;
>+
>+        newParagraph.appendChild(newSpan);
>+        document.body.appendChild(newParagraph);

Huh, why that complicated? Those nodes should be part of the HTML by default and just hidden by default. I wouldn't have expected that you create all of them on the fly. That's not necessary.
(In reply to Henrik Skupin (:whimboo) [away 07/27 - 08/05] from comment #19)
> Comment on attachment 646621 [details] [diff] [review]
> patch v8.1
> Huh, why that complicated? Those nodes should be part of the HTML by default
> and just hidden by default. I wouldn't have expected that you create all of
> them on the fly. That's not necessary.
If I remember correctly our IRC discussion, you wanted them created dynamically and not hidden by default. But again, I may not. So if you feel strong about that, I'll add a fix patch.
My wording was to show them only if a search term has been entered. So I really would like to see this happening before we are going on with the real Mozmill test. Would be great if you can come up with a follow-up patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch follow-up patch v1.0 (obsolete) — — Splinter Review 
Static elements that are shown on search
Attachment #649245 - Flags: review?(hskupin)
Attachment #649245 - Flags: review?(dave.hunt)
Comment on attachment 649245 [details] [diff] [review]
follow-up patch v1.0

>+  <p id="search-results">Search term was <span id="search-term"></span></p>

Just a nit, please add a ':' after 'was' or enclose the search term in brackets to make it easier to discover.
Attachment #649245 - Flags: review?(hskupin)
Attachment #649245 - Flags: review?(dave.hunt)
Attachment #649245 - Flags: review-
(In reply to Henrik Skupin (:whimboo) from comment #23)
> Comment on attachment 649245 [details] [diff] [review]
> follow-up patch v1.0
> 
> >+  <p id="search-results">Search term was <span id="search-term"></span></p>
> 
> Just a nit, please add a ':' after 'was' or enclose the search term in
> brackets to make it easier to discover.
The fact that it's bolded should accomplish that. Do we need both bold and brackets?
Sorry, meant quotes not brackets. So it's probably better to add the ': '.
added :
Attachment #649245 - Attachment is obsolete: true
Attachment #649670 - Flags: review?(hskupin)
Attachment #649670 - Flags: review?(hskupin) → review+
Landed:
http://hg.mozilla.org/qa/litmus-data/rev/aa006d68a7c3

Please double check the commit message before submitting reviews. The same guidelines also have to be followed for the litmus-data repository.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.