Closed Bug 1452533 Opened 6 years ago Closed 6 years ago

Change URL types to generate URL objects instead of nsIURIs

Categories

(Firefox :: Enterprise Policies, enhancement, P2)

Product:
Firefox
Component:
Enterprise Policies
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox-esr60 --- fixed
firefox62 --- fixed

People

(Reporter: Felipe, Assigned: kanika16047, Mentored)

References

Details

Attachments

(2 files)

Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs
59 bytes, text/x-review-board-request
Felipe
: review+
RyanVM
: approval-mozilla-esr60-
Details
Patch for ESR60
13.40 KB, patch
RyanVM
: approval-mozilla-esr60+
Details | Diff | Splinter Review 
Currently, the URL types supported by the policy engine (PoliciesValidator.jsm), which are "URL", "URLorEmpty" and "origin", automatically validates and convert the given string into an nsIURI object [1], which is an internal representation of URLs used by Firefox. 

However, it would be nicer to make the validator instead to generate the new-ish web-standard URL object [2], since they are more lightweight and they also look nicer in a JSON.stringify call.

They are very similar, but some fields have different names (e.g., .spec vs .href)

To get access to them in a .jsm file, you need to import the constructor, like this: [3]



[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIURI

[2] https://developer.mozilla.org/en-US/docs/Web/API/URL/URL

[3] https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/browser/components/enterprisepolicies/helpers/ProxyPolicies.jsm#9
Thank you! :)
Priority: -- → P5
Priority: P5 → P2
Blocks: 1465942
(In reply to :Felipe Gomes (back on Jun 5) from comment #0)
> Currently, the URL types supported by the policy engine
> (PoliciesValidator.jsm), which are "URL", "URLorEmpty" and "origin",
> automatically validates and convert the given string into an nsIURI object
> [1], which is an internal representation of URLs used by Firefox. 

Hi, I couldn't find any file named PoliciesValidator.jsm. 
Did you mean "mozilla-central/source/toolkit/components/utils/JsonSchemaValidator.jsm"? :)
Comment on attachment 8983544 [details]
Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs

https://reviewboard.mozilla.org/r/249396/#review255564


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/utils/JsonSchemaValidator.jsm:159
(Diff revision 1)
>  
>      case "origin":
>        if (typeof(param) != "string") {
>          break;
>        }
> +      

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]
Comment on attachment 8983544 [details]
Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs

(In reply to kanika16047 from comment #2)
> Hi, I couldn't find any file named PoliciesValidator.jsm. 
> Did you mean
> "mozilla-central/source/toolkit/components/utils/JsonSchemaValidator.jsm"? :)

Yep that's it, this file got renamed recently.


So there's a little bit more to do here.  The URL objects and nsIURI objects are a bit different. For example, on nsIURI we use .spec, and on URL we use .href.

So basically, you should search in policies-schema.json for all the parameter types specified with URL, and then follow how it's used inside each policy in Policies.jsm to adjust its usage.

There will be some tests to adjust too. At least https://searchfox.org/mozilla-central/source/toolkit/components/utils/test/browser/browser_JsonSchemaValidator.js will definitely need changes, but it's a good idea to run all the tests inside browser/components/enterprisepolicies folder.

And last but not least, we want the same change for the "origin" type.
Attachment #8983544 - Flags: review?(felipc)
(In reply to :Felipe Gomes (back on Jun 5) from comment #5)
> Comment on attachment 8983544 [details]
> Bug 1452533 - JsonSchemaValidator converts URL types into URL objects
> instead of nsIURIs
> 
> (In reply to kanika16047 from comment #2)
> > Hi, I couldn't find any file named PoliciesValidator.jsm. 
> > Did you mean
> > "mozilla-central/source/toolkit/components/utils/JsonSchemaValidator.jsm"? :)
> 
> Yep that's it, this file got renamed recently.
> 
> 
> So there's a little bit more to do here.  The URL objects and nsIURI objects
> are a bit different. For example, on nsIURI we use .spec, and on URL we use
> .href.
> 
> So basically, you should search in policies-schema.json for all the
> parameter types specified with URL, and then follow how it's used inside
> each policy in Policies.jsm to adjust its usage.
> 
> There will be some tests to adjust too. At least
> https://searchfox.org/mozilla-central/source/toolkit/components/utils/test/
> browser/browser_JsonSchemaValidator.js will definitely need changes, but
> it's a good idea to run all the tests inside
> browser/components/enterprisepolicies folder.
> 
> And last but not least, we want the same change for the "origin" type.

Thank you! :)
Comment on attachment 8983544 [details]
Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs

https://reviewboard.mozilla.org/r/249396/#review256828


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/utils/JsonSchemaValidator.jsm:159
(Diff revision 2)
>  
>      case "origin":
>        if (typeof(param) != "string") {
>          break;
>        }
> +      

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]
Right now 3 tests inside browser/components/enterprisepolicies are failing.
Bookmarks is the only policy which inputs URL object and is failing after the transition.

The other policies (origin objects) that are incompatible to this change are the ones that call addAllowDenyPermissions() inside which calls Services.perms.add and Services.perms.add throws error as it doesn't accept URL objects.
Hi Kanika,

for these cases that are failing, you can take the URL object given by the JsonSchemaValidator to the policy implementation, and then create an nsIURI object from it to pass it on (e.g. to the Services.perms.add call and to to calls made to the bookmarks/favicon services).
Comment on attachment 8983544 [details]
Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs

https://reviewboard.mozilla.org/r/249396/#review256916


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/tests/browser/browser_policy_clear_blocked_cookies.js:64
(Diff revision 3)
>  
>  add_task(function teardown() {
>    for (let host of [HOSTNAME_DOMAIN, ORIGIN_DOMAIN, "example.net"]) {
>      Services.cookies.removeCookiesWithOriginAttributes("{}", host);
>    }
>  });

Error: Newline required at end of file but not found. [eslint: eol-last]

::: toolkit/components/utils/JsonSchemaValidator.jsm:159
(Diff revision 3)
>  
>      case "origin":
>        if (typeof(param) != "string") {
>          break;
>        }
> +      

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]
Comment on attachment 8983544 [details]
Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs

https://reviewboard.mozilla.org/r/249396/#review257044
Attachment #8983544 - Flags: review?(felipc)
Comment on attachment 8983544 [details]
Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs

https://reviewboard.mozilla.org/r/249396/#review257190


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/tests/browser/browser_policy_clear_blocked_cookies.js:64
(Diff revision 4)
>  
>  add_task(function teardown() {
>    for (let host of [HOSTNAME_DOMAIN, ORIGIN_DOMAIN, "example.net"]) {
>      Services.cookies.removeCookiesWithOriginAttributes("{}", host);
>    }
>  });

Error: Newline required at end of file but not found. [eslint: eol-last]
Comment on attachment 8983544 [details]
Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs

https://reviewboard.mozilla.org/r/249396/#review257792

::: browser/components/enterprisepolicies/Policies.jsm:114
(Diff revision 4)
> +        param[index].URL = Services.io.newURI(param[index].URL.href);
> +        if (param[index].Favicon) {
> +          param[index].Favicon = Services.io.newURI(param[index].Favicon.href);
> +        }

Ok, let's try to simplify this..

I think the conversions can be done in a simpler way if done inside the BookmarksPolicies file.

In the lines where .spec is used (either URL.spec or Favicon.spec), you can just convert that to use .href

And then in the places where the actual object is used (bookmark.URL or bookmark.Favicon), you then create the newURI object just as a temporary variable, near where it will be used.

Two other things to note:

- Favicon.scheme will need to change to Favicon.protocol

- The big documentation comment at the beginning of BookmarksPolicies.jsm will need to be updated

::: browser/components/enterprisepolicies/Policies.jsm:495
(Diff revision 4)
> +      let Block = [];
> +      if (param.Allow) {
> +        for (let obj of param.Allow) {
> +          Allow.push(Services.io.newURI(obj.href));
> +        }
> +      }
> +      if (param.Block) {
> +        for (let obj of param.Block) {
> +          Block.push(Services.io.newURI(obj.href));
> +        }
> +      }

It'd be better to this conversion in addAllowDenyPermissions instead of having to manually do it here.

Inside the for loops there you can call the newURI function, and with that it won't be needed to create these temporary arrays.

::: toolkit/components/utils/JsonSchemaValidator.jsm:20
(Diff revision 4)
>  "use strict";
>  
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  
> +Cu.importGlobalProperties(["URL"]);

Please replace this line with
`XPCOMUtils.defineLazyGlobalGetters(this, ["URL"]);`

(it's a new function that was recently created in bug 1464548 and all new code should use it)
Attachment #8983544 - Flags: review?(felipc)
Comment on attachment 8983544 [details]
Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs

https://reviewboard.mozilla.org/r/249396/#review258010

Almost there!

::: browser/components/enterprisepolicies/Policies.jsm:130
(Diff revision 5)
> -        const hosts = param.Block.map(uri => uri.host).sort().join("\n");
> +        const hosts = param.Block.map(uri => Services.io.newURI(uri.href).host).sort().join("\n");
>          runOncePerModification("clearCookiesForBlockedHosts", hosts, () => {
>            for (let blocked of param.Block) {
> -            Services.cookies.removeCookiesWithOriginAttributes("{}", blocked.host);
> +            Services.cookies.removeCookiesWithOriginAttributes("{}", Services.io.newURI(blocked.href).host);

we don't need to convert these to nsIURI because these are only as strings.

the URL object contains both a .host and a .hostname property.

The URL.hostname matches what the nsIURI.host does, so let's just convert these two usages of .host to .hostname

(and rename the `uri` variable to `url`)

::: browser/components/enterprisepolicies/Policies.jsm:855
(Diff revision 5)
>   *        The list of URLs to be set as ALLOW_ACTION for the chosen permission.
>   * @param {array} blockList
>   *        The list of URLs to be set as DENY_ACTION for the chosen permission.
>   */
>  function addAllowDenyPermissions(permissionName, allowList, blockList) {
> +

nit: empty line added

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:213
(Diff revision 5)
>  async function setFaviconForBookmark(bookmark) {
>    let faviconURI;
>    let nullPrincipal = Services.scriptSecurityManager.createNullPrincipal({});
>  
> -  switch (bookmark.Favicon.scheme) {
> -    case "data":
> +  switch (bookmark.Favicon.protocol) {
> +    case "data:":

good catch!
Attachment #8983544 - Flags: review?(felipc)
Comment on attachment 8983544 [details]
Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs

https://reviewboard.mozilla.org/r/249396/#review258252
Attachment #8983544 - Flags: review?(felipc) → review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b40000a76390
JsonSchemaValidator should output URL types as URL objects instead of nsIURIs. r=felipe
https://hg.mozilla.org/mozilla-central/rev/b40000a76390
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Comment on attachment 8983544 [details]
Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs

This is needed so later Policy patches merge correctly.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Policy Work
User impact if declined: 
Fix Landed on Version: Firefox 62
Risk to taking this patch (and alternatives if risky): Low, policy only
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8983544 - Flags: approval-mozilla-esr60?
Comment on attachment 8983544 [details]
Bug 1452533 - JsonSchemaValidator converts URL types into URL objects instead of nsIURIs

*THIS* bug needs a rebased patch for ESR60 :)
Flags: needinfo?(mozilla)
Attachment #8983544 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60-
Besides needing bug 1455888, this patch happened after we moved the validator.

Once 1455888 is uplifted, I'll put a new patch together.
Blocks: 1479870
Attached patch Patch for ESR60 — — Splinter Review 
Sorry for the delay on this, Ryan.
Flags: needinfo?(mozilla)
Attachment #8996732 - Flags: approval-mozilla-esr60?
Comment on attachment 8996732 [details] [diff] [review]
Patch for ESR60

Policy engine fix needed on ESR60. Approved for ESR 60.2.
Attachment #8996732 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: qe-verify+
This bug was covered by the overall testing efforts invested in the New Enterprise Policies feature.

Adding the qe-verify- flag since this bug is covered by automation tests -https://docs.google.com/spreadsheets/d/123_gfQpKPH9_Xek8DV1v2HbT2dn3gC6KQcFHZ7v3U2g/edit?usp=sharing
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: