863139 - Add ui module for in-content security pages & security dialogs

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Andreea Matei [:AndreeaMatei]]

Given all the elements and different methods we've created in our testSecurity tests, it would be great to have them all compacted in a security.js library.

Comment 1

[Andrei Eftimie]

This should also replace the Lookup Expressions with findElement or nodeCollector

Comment 2

[Daniel Gherasim]

Attachment: new_securityInContentModule_v1.patch

Given that we are currently pressed with other work, I uploaded here what I have worked on this if someone else (maybe a contributor) would like to continue.

So here is the discription for the library I have created:

We create different classes for every security related UI element/window to handle it.

So we now have the following classes with methods:

* securityTab
** getElement
** getElements

* securityWarningDialog
** getElement
** getElements

* downloadingCertificateDialog
** getElement
** getElements
** getTrustElements

* securityExceptionDialog
** getElement
** getElements

* securityUI
** get certificate
** getCertificateProperty
** getProperty
** getLocation
** getEntity
** getElement
** getElements

All this and the patch from bug 816084 should replace all id's and a lot of code we constantly repeat in our security tests.

Comment 3

[Daniel Gherasim]

Attachment: new_securityInContentModule_v1.1.patch

Updated patch for this refactoring.
I worked on this a while ago and I think we can start the proccess to apply this for a better structure of our code.

Comment 4

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8392109 [details] [diff] [review]
new_securityInContentModule_v1.1.patch

Review of attachment 8392109 [details] [diff] [review]:
-----------------------------------------------------------------

If you need feedback, you can start with that only.

::: firefox/lib/security.js
@@ +51,5 @@
> +   *                             [optional - default: ""]
> +   *                  value    - Value of the attribute to filter
> +   *                             [optional - default: ""]
> +   *                  parent   - Parent of the to find element
> +   *                             [optional - default: document]

We use all these with @param tag and the optional values have another style e.g [optional_elem=default_value]

@@ +73,5 @@
> +   *                             [optional - default: ""]
> +   *                  value    - Value of the attribute to filter
> +   *                             [optional - default: ""]
> +   *                  parent   - Parent of the to find element
> +   *                             [optional - default: document]

Same here.

@@ +86,5 @@
> +    var value = spec.value;
> +    var parent = spec.parent;
> +
> +    var root = parent ? parent.getNode() : this._controller.window.document;
> +    var nodeCollector = new domUtils.nodeCollector(root);

I don't see this being used.

@@ +90,5 @@
> +    var nodeCollector = new domUtils.nodeCollector(root);
> +
> +    switch(spec.type) {
> +      case "securityTab":
> +        return [new elementslib.ID(root, "securityTab")];

Please keep the format as in all the other libraries.

@@ +225,5 @@
> +  getTrustElements : function downloadingCertificateDialog_getTrustElements() {
> +    var trustElementsIDS = ["trustSSL", "trustEmail", "trustObjSign"];
> +    var trustElements = [];
> +
> +    for(var index in trustElementsIDS) {

I would use forEach() here.

@@ +288,5 @@
> +        nodeCollector.root = nodeCollector.nodes[0];
> +        nodeCollector.queryAnonymousNode("dlgtype", "accept");
> +        break;
> +      case "trustSSL":
> +        return [new elementslib.ID(root, "trustSSL")];

When there are cases with both IDs and nodeCollector, we can use nodeCollector for the elements with IDs (queryNodes), so we'll only return in after the switch.

@@ +489,5 @@
> +        break;
> +      case "technicalContentText":
> +        return [new elementslib.ID(root, "technicalContentText")];
> +        break;
> +      case "cert_domain_link":

Please sort the elements alphabetically.

@@ +509,5 @@
> + *
> + * @returns {Object} Certificate of the current browsing page
> + */
> +function getCertificate(aController) {
> +  if(!aController)

Whitespace after if and brackets please.

@@ +569,5 @@
> + * @returns {String} The value of the requested property
> + */
> +function getProperty(aPrefName) {
> +  var prop = null;
> +  PROPRIETES_FILES.forEach(function(url) {

We should use => where needed.

::: firefox/tests/functional/testSecurity/testGreyLarry.js
@@ +61,5 @@
>  function checkSecurityTab(controller) {
> +  securityTab = new security.securityTab(controller);
> +
> +  var securityTabEl = securityTab.getElement({type: "securityTab"});
> +  expect.ok(securityTabEl.getNode().selected, "The Security tab is selected by default");

Where the lines exceed 80 chars please move the message on another one.

Comment 5

[Daniel Gherasim]

Attachment: new_securityLibrary_v1.0.patch

Thanks for the review, I've updated the patch now.

Comment 6

[Daniel Gherasim]

The library is not just about in-content elements, security dialogs with their elements will be handled too.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8392109 [details] [diff] [review]
new_securityInContentModule_v1.1.patch

Review of attachment 8392109 [details] [diff] [review]:
-----------------------------------------------------------------

Right direction but still a couple of things to do before it will get my r+.

::: firefox/lib/security.js
@@ +19,5 @@
> +  "chrome://pipnss/locale/security.properties"
> +  ];
> +
> +/**
> + * Security Tab Manager class

It's not clear to me what this class stands for.

@@ +26,5 @@
> + * @param {MozMillController} aController
> + *        MozMillController of the Security Tab
> + */
> +function securityTab(aController) {
> +  this._controller = aController;

You might want to assert that you have a valid controller.

@@ +110,5 @@
> +  }
> +};
> +
> +/**
> + * Security Warning Dialog Manager class

Same here. Try to find out the correct name via the window type.

@@ +183,5 @@
> +    switch(spec.type) {
> +      case "okButton":
> +        nodeCollector.queryNodes("#commonDialog");
> +        nodeCollector.root = nodeCollector.nodes[0];
> +        nodeCollector.queryAnonymousNode("dlgtype","accept");

We might want to create the ui module for the commonDialog first, so we could subclass this class? There is a bug open for it already.

@@ +200,5 @@
> + * @constructor
> + * @param {MozMillController} aController
> + *        MozMillController of the Downloading Certificate Dialog
> + */
> +function downloadingCertificateDialog(aController) {

Classes start with a capital letter

@@ +221,5 @@
> +   *
> +   * @returns List of nodes
> +   * @type {array of ElemBase}
> +   */
> +  getTrustElements : function downloadingCertificateDialog_getTrustElements() {

Do we really need this method? I don't think so.

@@ +389,5 @@
> +  },
> +};
> +
> +/**
> + * Security UI manager class

Not sure what this stands for.

@@ +518,5 @@
> +
> +  if(SSLStatus === null)
> +    return null;
> +
> +  return SSLStatus.serverCert;

This is backend code.

@@ +544,5 @@
> +      return aCert.subjectName.substring(aCert.subjectName.indexOf("ST=") + 3,
> +                                         aCert.subjectName.indexOf(",C="));
> +    default :
> +      assert.fail("Unknown property given - " + aPrefName);
> +  }

Same here.

@@ +576,5 @@
> +    }
> +    catch(ex) {}
> +  });
> +  return prop;
> +};

Both methods above have to be part of the ui classes.

@@ +596,5 @@
> +  var locationLabel = getProperty("identity.identified.state_and_country");
> +  var updateLocationLabel = locationLabel.replace("%S", state).replace("%S", country);
> +  var location = city + '\n' + updateLocationLabel;
> +  return location;
> +};

back-end code.

::: firefox/tests/functional/testSecurity/testGreyLarry.js
@@ +14,5 @@
>  
>  var setupModule = function(aModule) {
>    aModule.controller = mozmill.getBrowserController();
> +
> +  aModule.securityTab = null;

I dont see why it has to be defined here.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +29,5 @@
>    controller.open(TEST_DATA);
>    controller.waitForPageLoad();
>  
>    // Get the information from the certificate
> +  cert = security.getCertificate(controller);

You are defining a global variable here which might leak.

::: firefox/tests/remote/testSecurity/testMD5HashSignature.js
@@ +36,5 @@
>  
>    controller.open(TEST_DATA.url_page);
>    controller.waitForPageLoad();
>  
> +  var expertContentHeading = securityUI.getElement({type: "expertContent"});

So is that a test page or an  in-content chrome page? For the latter it should be fine but we do not want to write ui libraries for remote or test pages.

Comment 8

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8392870 [details] [diff] [review]
new_securityLibrary_v1.0.patch

Review of attachment 8392870 [details] [diff] [review]:
-----------------------------------------------------------------

This has to be updated after the feedback received.

Comment 9

[Daniel Gherasim]

According to Henrik's feedback, I suggest we make the following refactoring in our code:

In firefox/lib/ui : 

-> page-info.js
    -> securityTab class;
    -> Also the other tabs could be added in the future if needed;
-> security.js
    -> WarningDialog class
    -> CertificateDialog class
    -> ExceptionDialog class
    -> InContentPages - In content static pages to handle elements like the getMeOutOfHere, reportButton buttons and so on.

In firefox/lib (back-end code):
-> security.js
    -> getCertificate
    -> getCertificateProperty
    -> getSecurityProperty 
    -> getCertificateLocation

What do you think of this ?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to daniel.gherasim from comment #9)
> -> page-info.js
>     -> securityTab class;
>     -> Also the other tabs could be added in the future if needed;

There will be no securityTab class as entry point in that module. We will have a PageInfoWindow class, which contains tabs. One of those is the security one. We may split this out as a class, but not export it.

> -> security.js
>     -> WarningDialog class
>     -> CertificateDialog class
>     -> ExceptionDialog class

Do all of those have their own window type? If yes we might want to implement separate top-level ui modules.

>     -> InContentPages - In content static pages to handle elements like the
> getMeOutOfHere, reportButton buttons and so on.

This may get its own module, and hold all available in-content pages.

> In firefox/lib (back-end code):
> -> security.js
>     -> getCertificate
>     -> getCertificateProperty
>     -> getSecurityProperty 
>     -> getCertificateLocation

Is this Firefox only? I think it can be in the top-level lib folder.

Comment 11

[Daniel Gherasim]

Attachment: new_securityLibrary_v2.0.patch

According to our discussion I have created the requested files/libraries.

/firefox/lib/ui/certificate-dialog.js
/firefox/lib/ui/exception-dialog.js
/firefox/lib/ui/in-content-pages.js
/firefox/lib/ui/page-info.js (securityTab class).
/firefox/lib/ui/warning-dialog.js
/lib/security.js

Comment 12

[Andrei Eftimie]

Comment on attachment 8403896 [details] [diff] [review]
new_securityLibrary_v2.0.patch

Review of attachment 8403896 [details] [diff] [review]:
-----------------------------------------------------------------

There are still some things that we need to address.

And right now there's a conflict in testSafeBrowsingNotificationBar.js

::: firefox/lib/ui/certificate-dialog.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + "use strict";

nit: extra whitespace

@@ +80,5 @@
> +        nodeCollector.root = nodeCollector.nodes[0];
> +        nodeCollector.queryAnonymousNode("dlgtype", "accept");
> +        break;
> +      case "trustSSL":
> +        nodeCollector.queryNodes("#trustSSL");

This doesn't look like it needs NodeCollector.
We should use findElement where appropriate.

@@ +83,5 @@
> +      case "trustSSL":
> +        nodeCollector.queryNodes("#trustSSL");
> +        break;
> +      case "trustEmail":
> +        nodeCollector.queryNodes("#trustEmail");

Same here.

@@ +86,5 @@
> +      case "trustEmail":
> +        nodeCollector.queryNodes("#trustEmail");
> +        break;
> +      case "trustObjSign":
> +        nodeCollector.queryNodes("#trustObjSign");

And here.

::: firefox/lib/ui/exception-dialog.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + "use strict";

nit: extra whitespace

@@ +75,5 @@
> +    var nodeCollector = new domUtils.nodeCollector(root);
> +
> +    switch(spec.type) {
> +      case "certificateButton":
> +        nodeCollector.queryNodes("#checkCertButton");

Same here. findElement should suffice.

::: firefox/lib/ui/in-content-pages.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + "use strict";

nit: extra whitespace

@@ +83,5 @@
> +    var document = this._controller.window.document;
> +
> +    switch(spec.type) {
> +      case "cert_domain_link":
> +        elem = new elementslib.ID(document, "cert_domain_link");

We should use findElement instead of directly calling elementslib.

::: firefox/lib/ui/page-info.js
@@ +74,5 @@
> +    var elem = null;
> +
> +    switch(spec.type) {
> +      case "general":
> +        elem = new elementslib.ID(this._controller.window.document, "generalTab");

findElement please

@@ +161,5 @@
> +    var elem = null;
> +
> +    switch(spec.type) {
> +      case "domain":
> +        elem = new elementslib.ID(this._controller.window.document,

Also findElement

::: firefox/lib/ui/warning-dialog.js
@@ +64,5 @@
> +   *        Value of the attribute to filter
> +   * @param {String} [aSpec.parent=document]
> +   *        Parent of the to find element
> +   *
> +   * @returns  {[ElemBase]}Elements which have been found

nit: some spacing issues here

@@ +70,5 @@
> +  getElements: function warningDialog_getElements(aSpec) {
> +    var spec = aSpec || { };
> +    var parent = spec.parent;
> +
> +    var elems = null;

We should keep the convention and use `elem` as a name as we do in all other libs.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +98,5 @@
>   */
> +function checkSecurityTab(aController) {
> +  var pageInfoWindow = new pageInfo.pageInfoWindow(aController);
> +
> +  var securityTabEl = pageInfoWindow.getElement({type: "security"});

Why not keep the old name `securityTab`?
I don't think `El` is providing any benefit.

@@ +118,3 @@
>    expect.equal(webIDVerifierLabel.getNode().value, cert.issuerOrganization,
>                 "Expected verifier label found");
> +  aController.keypress(null, 'VK_ESCAPE', {});

We should add a close() method to all `panels` and call that in relevant tests.
The method should probably also assert that the panel has actually closed.

::: firefox/tests/remote/testSecurity/testGreenLarry.js
@@ +119,5 @@
>   */
> +function checkSecurityTab(aController) {
> +  var pageInfoWindow = new pageInfo.pageInfoWindow(aController);
> +
> +  var securityTabEl = pageInfoWindow.getElement({type: "security"});

I would keep the original name `securityTab`

::: firefox/tests/remote/testSecurity/testMD5HashSignature.js
@@ +33,1 @@
>    var expertContentButton = expertContentHeading.getNode().childNodes[1];

This should also be retrieved through `icPages.getElement()`.
It would be even better if we can use a selector instead of using `childNodes`.

And since the `expertContentHeading` is _only_ used in this place, we might replace that case entirely with this one.

::: firefox/tests/remote/testSecurity/testUnknownIssuer.js
@@ +22,5 @@
>    controller.waitForElement(link);
>    expect.equal(link.getNode().textContent, "ssl-selfsigned-unknownissuer.mozqa.com",
>                 "Domain name is visible");
>    // Verify "Get Me Out Of Here!" button appears
> +  var  getMeOutOfHereButton = icPages.getElement({type: "getMeOutOfHereButton"});

nit: extra whitespace

::: lib/security.js
@@ +20,5 @@
> +
> +  var securityUI = aController.window.getBrowser().mCurrentBrowser.securityUI;
> +  var SSLStatus = securityUI.QueryInterface(Ci.nsISSLStatusProvider).SSLStatus;
> +
> +  if (SSLStatus === null)

Please always use curly brackets.

You could also use the ternary operator and only have 1 return statement.

@@ +62,5 @@
> + *        The property to get the value of
> + *
> + * @returns {String} The value of the requested property
> + */
> +function getCertificateLocation(aCert, aLocationProp) {

I'm wondering if this should be a case in `getCertificateProperty`.
It is not a direct property, but a computed one.

How does a case for `location` sound instead of a new method altogether?

Comment 13

[Daniel Gherasim]

Making this depended on bug 816084 given that we'll make some changes that will affect and get disabled severel tests also modified here.

Comment 14

[Daniel Gherasim]

^ enabled, sorry!

Comment 15

[Daniel Gherasim]

Attachment: new_securityLibrary_v3.0.patch

Thanks Andrei, patch updated.

Comment 16

[Andrei Eftimie]

Comment on attachment 8408919 [details] [diff] [review]
new_securityLibrary_v3.0.patch

Review of attachment 8408919 [details] [diff] [review]:
-----------------------------------------------------------------

This patch will have to be updated once bug 994040 lands. I'll take of the review flag until then.

Comment 17

[Daniel Gherasim]

Bug 994040 was fixed but this patch touches almost the same files as in bug 816084. We'll wait for that to be finished & continue afterwards on this.

Comment 18

[Daniel Gherasim]

We are implementing the page-info class in Bug 1055453!

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Mozmill is dead, WONTFIX the remaining bugs.