1269195 - Data Manager enhancements for non domain permissions and preferences

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Frank-Rainer Grahl (:frg)]

The Data Manager will currently not display and process all permissions and preferences correctly. Only permissions with a common scheme and preferences for a valid host are supported. Others like chrome://, file:// and for internal pages like about: will either be displayed as a blank domain or with an arbitrary host. 

This needs to be fixed.

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1269195-DataManagerEnhancement.patch

Moves all non standard hosts and schemes to the * global data domain for permissions and preferences.

Tested with various schemes eg. chrome:// and files:// and internal pages like setting zoom factor for about:blank

Comment 2

[Philip Chee]

Comment on attachment 8747527 [details] [diff] [review]
1269195-DataManagerEnhancement.patch

r=me with the following fixed:

In the "*" domain I have file:/// mailbox:/// about:
In the ":" domain I have jar:/ <- Shouldn't this go under "*"

Did you test for resource: and gopher: ?

> -        while (statement.executeStep())
> -          gDomains.addDomainOrFlag(statement.row["host"], "hasPreferences");
> +        while (statement.executeStep()) {
> +          gDataman.debugMsg("Found pref: " + statement.row["host"]);
> +          var prefHost = gDomains.getDomainFromHostWithCheck(statement.row["host"]);
To be consistent. use "let" in scoped blocks if/do/while/try/catch

> +  getDomainFromHostWithCheck: function domain_getDomainFromHostWithCheck(aHost)  {
> +    let host = gDomains.getDomainFromHost(aHost);
Maybe: let host = gDomains.getDomainFromHost(aHost).trim();

> +    if ((host.trim().length == 0) ||
> +        (aHost.slice(0,6) == "about:"))
Is this a string? If so you can use aHost.startsWith("about:");

Due to operator precedence you don't need the internal ()'s so just use:
if (!host.length || aHost.startsWith("about:"))

> +  // Used for checking if * global data domain should be used.
> +  commonScheme: function domain_commonScheme(aScheme) {
> +    if (aScheme != "http" &&
> +        aScheme != "https" &&
> +        aScheme != "ftp") {
> +      return false;
>      }
> -    return this.xlcache[aHostname];
> +
> +    return true;
>    },
Just use (SeaMonkey coding style):
return /^(https?|ftp):/i.test(aScheme);

Comment 3

[Frank-Rainer Grahl (:frg)]

>> In the ":" domain I have jar:/ <- Shouldn't this go under "*"

You found another case where the prefs api did put the full url into the group pref as a host. This needs the same static treatment as about: or should I try to parse the host?

>> Did you test for resource: and gopher: ?

Now I did:) resource seems to work ok. I didn't think the gopher api was still supported by the permission manager but with the overbite add-on a valid permission can be added. Didn't use gopher in ages. I need to put gopher in the commonScheme function or the pref host ends up in the domains and the permission in *. There are maybe others but I can't think fo them right now.

Will fix the other remarks. Ok with these additional changes or should I set the review again when I am finished?

Comment 4

[Philip Chee]

(In reply to Frank-Rainer Grahl from comment #3)
> >> In the ":" domain I have jar:/ <- Shouldn't this go under "*"
> 
> You found another case where the prefs api did put the full url into the
> group pref as a host.

jar: can point to a local file. It can also point to a remote http://
There are a couple of prefs you need to set.

network.jar.block-remote-files;false
network.jar.open-unsafe-types;true

> This needs the same static treatment as about: or
> should I try to parse the host?

I don't think the jar: protocol has the concept of "host".
jar:file:///tmp/simple.blah!/
jar:http://www.skierpage.com/moz_bugs/simple.myarchive!/

> Will fix the other remarks. Ok with these additional changes or should I set
> the review again when I am finished?
I would like a quick peek at your additional changes. Thanks.

Comment 5

[Philip Chee]

> return /^(https?|ftp):/i.test(aScheme);
Of course, that should be without the ":". Lower-casing might not be necessary either.
return /^(https?|ftp)/i.test(aScheme);

Comment 6

[Frank-Rainer Grahl (:frg)]

Attachment: 1269195-DataManagerEnhancement-V2.patch

Hopefullly I have covered everything for now. Remote xul also tested.

Comment 7

[Philip Chee]

Comment on attachment 8750108 [details] [diff] [review]
1269195-DataManagerEnhancement-V2.patch

r=me

Comment 8

[Frank-Rainer Grahl (:frg)]

Checked in:
https://hg.mozilla.org/comm-central/rev/7c5932e3a260

Comment 9

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8750108 [details] [diff] [review]
1269195-DataManagerEnhancement-V2.patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Domain view faulty. Some permissions and preferences will not be shown under the correct domain or not at all.
Testing completed (on m-c, etc.): c-a
Risk to taking this patch (and alternatives if risky): None. Tested
String changes made by this patch: none

Comment 10

[Philip Chee]

Comment on attachment 8750108 [details] [diff] [review]
1269195-DataManagerEnhancement-V2.patch

a=me for comm-aurora

Comment 11

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-aurora/rev/814f87ba88b5