270170 - XPInstall Permission Manager UI

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

This is the front end component of bug 240552. Provide a UI for manipulating the
list of whitelisted install sites.  This is the seamonkey version of firefox bug
241705.

Comment 1

[Robert Kaiser]

We badly would need that, as failing silently when the user clicks on a valid
link is very bad. We should at least show the standard download dialog if we
don't install the linked file.

BTW, it would be quite nice anyways to have the possibility to save an XPI to
disk rather than installing it...

Comment 2

[Ian Neal]

Attachment: Provisional pass v0.1

This patch is mainly a port of CookieExceptions.xul/js code with a few bits
thrown in. Image, Popup and Install Managers hook into it at the moment.

I'm using popupManager.xul/js as the filenames currently so I don't have to
mess around with adding files to cvs and is easier to apply for testing
purposes.

I've done a fair amount of testing and but there could still be some bugs in
there. You will also need to apply the patch to bug 272764 to get xpi
installation fully working.

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Removing dependency on bug 270443, as Ian's implementation doesn't need it.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

I'm not sure what the status of the patch here is, but extension/cookie should
not be overloaded even more. I know it is a mess there, but don't make the mess
any bigger.

Comment 5

[[e-mail]]

(In reply to comment #4)
>I'm not sure what the status of the patch here is, but extension/cookie should
>not be overloaded even more. I know it is a mess there, but don't make the mess
>any bigger.
Bigger? It seems to me that this stuff is taking even more out of cookie; the
cookie manager already lost its popup manager rôle two years ago, and never got
cleaned up afterwards; now if Ian is unifying the XPI, popup and image managers
this will mean that the cookie manager will only manage cookies, a good thing,
surely? Some of the functions in the overlay might need new homes though.

Comment 6

[Ian Neal]

I have been working on a patch to tidy up some of the cookie js/xul/pref code
and will attach it to bug 277097 as I don't want delay getting the install stuff in.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #5)
> Bigger? It seems to me that this stuff is taking even more out of cookie
Sorry, I misread the patch. I only looked at the first index: line. never mind :)

Comment 8

[Asa Dotzler [:asa]]

I don't think we'd block the release of 1.8a6 on this bug. If the bug or patch
owner disagrees, please renominate. Thanks.

Comment 9

[[e-mail]]

Comment on attachment 170001 [details] [diff] [review]
Provisional pass v0.1

>+    var params = { blockVisible: (viewerType == "image"),
>+                   sessionVisible: false,
>+                   allowVisible: true,
>+                   prefilledHost: host,
>+                   permissionType: viewerType };
I think sessionVisible only applies to the cookie viewer, which I assume you're
not going to try and merge back in, are you? In which case you can excise all
the session permissions from the expanded permission manager.

>-    <data id="prefillWhitelist" preftype="bool" prefattribute="value" prefstring="privacy.popups.prefill_whitelist"/>
>-    <data id="removeBlacklist" preftype="bool" prefattribute="value" prefstring="privacy.popups.remove_blacklist"/>
Why aren't we still using these?

>Index: xpfe/communicator/resources/content/popupManager.js
I can't review this as a diff, it's too messy. Please could you attach the new
version of this file on its own.

Comment 10

[Ian Neal]

Attachment: popupManager.js

Comment 11

[Ian Neal]

Attachment: popupManager.xul

Comment 12

[Ian Neal]

Attachment: Revised patch including prefill/remove v0.1a

Changes since v0.1
* Put back missing prefill whitelist and remove backlist

Comment 13

[Ian Neal]

Attachment: popupManager.js 01_08

Revised popupManager.js

Comment 14

[Ian Neal]

Attachment: popupManager.xul 01_08

Revised popupManager.xul

Comment 15

[Ian Neal]

Attachment: Updated popupManager patch v0.1b

Changes since v0.1b
* updated original version of popupManager.js to have the functionality of ff's
cookieExceptions.js
* persists sort order

Comment 16

[Ian Neal]

Attachment: revised popupManager patch v0.1c

Changes since v0.1b
* Replicated changes from bug 259126
* Changed .number -> .id in nsWalletTreeUtils.js and its users

Comment 17

[Ian Neal]

Attachment: popupManager for installs v0.1d

Changes since v0.1c
* Changed to using .textContent for permissionsText instead of adding children

Comment 18

[Ian Neal]

Attachment: popupManager for images/installs too v0.1e

Changes since v0.1d
* Missed two .number -> .id changes

Comment 19

[Ian Neal]

mvl is this something you could review?

Comment 20

[Michiel van Leeuwen (email: mvl+moz@)]

This patch keeps on confusing me. Adding stuff that already is in the cookie
manager to the popupmanager, which now does more then managing popups. I'd
rather see the files moved and renamed first, before editing them again. Then
you don't need weird stuff like having the xpinstall prefs panel include cookie
javascript files.


> * Changed .number -> .id in nsWalletTreeUtils.js and its users

Why? It just makes the patch a lot bigger with not much gain. It looks a tiny
but better, but not much. And it certainly doesn't do anything to fix this bug.

Comment 21

[timeless]

Comment on attachment 170784 [details] [diff] [review]
popupManager for images/installs too v0.1e

>Index: extensions/cookie/resources/content/cookieNavigatorOverlay.xul
>+                  oncommand="viewPopups(getBrowser().currentURI.host);"/>
you should use a function, .host can throw.

>Index: extensions/cookie/resources/content/pref-popups.xul
>       function loadWhitelist() {
>         try {
>+          var whitelist = parent.hPrefWindow.getPref("localizedstring",
>+                                                     "privacy.popups.default_whitelist");
>+          var hosts = whitelist.split(",");
> 
>           for (var i = 0; i < hosts.length; i++) {
>             var host = hosts[i];
>             host = "http://" + host;
>             var uri = ioService.newURI(host, null, null);
>             permissionManager.add(uri, popupType, true);

this code doesn't seem very friendly to https urls, but i suppose that's ok.

>Index: xpfe/browser/resources/content/navigator.js
>@@ -2309,18 +2309,17 @@ function getBrowserForDocument(doc) {  
> function StatusbarViewPopupManager() {
>   var hostPort = "";
>   try {
>     hostPort = getBrowser().selectedBrowser.currentURI.hostPort;
>   }
>   catch(ex) { }
>   
>   // open whitelist with site prefilled to unblock
>-  window.openDialog("chrome://communicator/content/popupManager.xul", "",
>-                      "chrome,resizable=yes", hostPort);
>+  viewPopups(hostPort);
> }

your other code used .host, not .hostPort, but note that this code uses a try
block.

>Index: xpfe/communicator/resources/content/popupManager.js
> function Startup() {
>   permissionManager = Components.classes["@mozilla.org/permissionmanager;1"]
>+                                .getService(nsIPermissionManager);
if this thows, what happens?

i presume there's some check to handle arguments.length == 0, if there isn't,
please add one.
>+  document.getElementById("btnBlock").hidden = !window.arguments[0].blockVisible;
>+  document.getElementById("btnSession").hidden = !window.arguments[0].sessionVisible;
>+  document.getElementById("btnAllow").hidden = !window.arguments[0].allowVisible;

> function onAccept() {
>+function loadPermissions() {
>   var enumerator = permissionManager.enumerator;
>   var count = 0;
>+  var permission;
>+
>   while (enumerator.hasMoreElements()) {
>+    permission = enumerator.getNext().QueryInterface(Components.interfaces.nsIPermission);
is fatally throwing the right behavior if getNext or QueryInterface fail?
I believe it isn't.

>+    if (permission.type == permissionType)
>+      permissionPush(count++, permission.host, permission.type,
>+                     capabilityString(permission.capability), permission.capability);
>   }
> function finalizeChanges() {

>+  for (i = 0; i < removals.length; ++i) {
>+    p = removals[i];
i think i'd like a try block around this:
>+    permissionManager.remove(p.host, p.type);
>   }
> 
>+  for (i = 0; i < additions.length; ++i) {
>+    p = additions[i];
>+    uri.spec = p.host;
and this:
>+    permissionManager.add(uri, p.type, p.perm);
>+  }
> }

>Index: xpfe/communicator/resources/content/popupManager.xul
to be continued

Comment 22

[Ian Neal]

(In reply to comment #21)
> >Index: extensions/cookie/resources/content/pref-popups.xul
> >       function loadWhitelist() {
> >         try {
> >+          var whitelist = parent.hPrefWindow.getPref("localizedstring",
> >+                                                    
"privacy.popups.default_whitelist");
> >+          var hosts = whitelist.split(",");
> > 
> >           for (var i = 0; i < hosts.length; i++) {
> >             var host = hosts[i];
> >             host = "http://" + host;
> >             var uri = ioService.newURI(host, null, null);
> >             permissionManager.add(uri, popupType, true);
> 
> this code doesn't seem very friendly to https urls, but i suppose that's ok.
True, could add https as well but until default_whitelist can indicate what sort
of url it is, not much else can be done that I can think of at the moment.

> >Index: xpfe/communicator/resources/content/popupManager.js
> > function Startup() {
> >   permissionManager = Components.classes["@mozilla.org/permissionmanager;1"]
> >+                                .getService(nsIPermissionManager);
> if this thows, what happens?
Basically, nothing in the site manager UI works and lots of errors are generated
in the JS console. Couple of options off the top of my head are:
a) Disable everything but the cancel button - no errors but user isn't told what
is wrong?
b) Test before trying to bring up the site manager UI and bring up a user
visible error message.
> 
> i presume there's some check to handle arguments.length == 0, if there isn't,
> please add one.
There isn't a check but all callers, as far as I can see, provide arguments -
but can check, just have to work out what to do if there aren't any.

All the other comments should be fairly straight forward to address so far.

Comment 23

[Ian Neal]

Attachment: Revised permissionsManager for images/installs too v0.2

Changes in this patch
* Unbitrotted against current tree
* Takes on board reviewer's comments

Comment 24

[Ian Neal]

Attachment: Unbitrotted patch v0.2a

Unbitrotted version of patch v0.2

Comment 25

[Daniel Brooks [:db48x]]

Comment on attachment 187252 [details] [diff] [review]
Unbitrotted patch v0.2a

>+  try {
>+    permissionManager = Components.classes["@mozilla.org/permissionmanager;1"]
>+                                  .getService(nsIPermissionManager);
>+  } catch(ex) {
>+  }

Under what circumstances does this fail? I'd say that if this fails you should
disable everything but cancel, but I suppose it also depends on what happens to
the rest of the browser in that case. I'm not as familiar with this area as I'd
like to be.

> function handlePermissionKeyPress(e) {
>   if (e.keyCode == 46) {
>     deletePermissions();
>   }
> }

I know that this part of the code is preexisting, but that ought to use a <key>
so that derivitive products can change the keybindings without having to edit
javascript code. There's no need to hold this patch up for that though.

>+  var host = textbox.value.replace(/^\s*([-\w]*:\/+)?/, ""); // trim any leading space and scheme

I don't understand why you're removing the scheme and then adding http back -
it seems like you're creating an artificial limitation. Why not test that the
scheme is within the allowed subset of possible schemes? Would you want to
allow an ftp page to install software? If not, why not?

>+  // check whether the permission already exists, if not, add it
>+  var exists = false;
>+  for (var i = 0; i < additions.length; ++i) {
>+    if (additions[i].rawHost == host) {
>+      exists = true;
>+      additions[i].capability = stringCapability;
>+      additions[i].perm = aPermission;
>+      break;
>     }

use the in operator, it makes for a cleaner loop, and it's used elsewhere in
the file.

Other than that it looks good. Everything seems to work too.

Comment 26

[Robert Kaiser]

I'd really really love to have this in SeaMonkey, but I don't believe we're
holding SeaMonkey 1.0 Alpha for this. Please consider to re-nominate for Beta
once Alpha is out - but I hope this patch will get reviews and go in even before
that is necessary.

BTW, what holds the patch from being marked r+ currently?

Comment 27

[Ian Neal]

Attachment: Updated patch v0.2b

Changes since v0.2a
* If permissionManager fails then only cancel button is available
* Used (var i in foo) instead of (var i = 0; i < foo.length; i++) in various
for loops

From what I understand of the code the other changes cannont be made because
the permissionManager backend does not understand schemes.

Carrying forward implicit r= and requesting sr=

Comment 28

[jag (Peter Annema)]

Comment on attachment 192373 [details] [diff] [review]
Updated patch v0.2b

>Index: xpfe/components/permissions/content/permissionsManager.js
>===================================================================

>+  try {
>+    permissionManager = Components.classes["@mozilla.org/permissionmanager;1"]
>+                                  .getService(nsIPermissionManager);
>+  } catch(ex) {
>+  }

Why would this fail?


>+  var win;
>+  var browsers;
>+  var i;
>+  var j;

I would've kept/put these vars inline.


>+  while (enumerator.hasMoreElements()) {
>+    win = enumerator.getNext();
>+
>+    browsers = win.getBrowser().browsers;
>+    for (i in browsers) {
>       try {
>         nextLocation = browsers[i].currentURI.hostPort;
>       }
>       catch(ex) { 
>         //blank window
>       }
> 
>       if (nextLocation) {
>         nextLocation = '.'+nextLocation;

(Not new, but): If that try/catch fails, |nextLocation| will have its previous value, which I don't think is what you want.


>+function handleHostInput(aSiteField) {
>+  // trim any leading space and set buttons appropiately
>+  btnDisable(!aSiteField.value.replace(/^\s*([-\w]*:\/+)?/, ""));
> }

Not sure I like this custom scheme match, but I guess since it's what's used elsewhere we could turn this worry into a new bug. You might also want to disable when the value is "   http://   ".


> function finalizeChanges() {
>+  var uri = Components.classes["@mozilla.org/network/standard-url;1"]
>+                      .createInstance(Components.interfaces.nsIURI);

The proper way to construct a URI object is through IOService, as it was done before.


>+function addPermission(aPermission) {
>+  var textbox = document.getElementById("url");
>+  var host = textbox.value.replace(/^\s*([-\w]*:\/+)?/, ""); // trim any leading space and scheme
>+  try {
>     var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>                               .getService(Components.interfaces.nsIIOService);
>+    var uri = ioService.newURI("http://"+host, null, null);
>+    host = uri.host;

>+    if (host == "" || host.replace(/\W/, "") == "")
>+      throw("Error: Invalid hostname entered.");

Doesn't |newURI()| throw if "http://"+host is invalid? In which case I doubt host would be empty or whitespace only, so that test seems redundant.

Also, nit: spaces around +

Comment 29

[Ian Neal]

Attachment: Revised patch addressing sr comments v0.2c

Changes since v0.2b:
* Removed try/catch from round permissionManager setting
* Inlined some var declarations
* Set nextLocation to null if currentURI errors
* Created new function to trim leading/trailing spaces and scheme
* Constructs uri using IOService
* Fixed space nits

carrying forward r= and requesting sr=

Comment 30

[Ian Neal]

Attachment: Update to help patch v0.1

This patch:
* Updates pref panel help to reflect updated software installation prefs

I have noticed that images pref panel help does not mention Manage Image Permissions Button but that should probably be a separate bug (if it is not already one).

Comment 31

[jag (Peter Annema)]

Comment on attachment 213809 [details] [diff] [review]
Revised patch addressing sr comments v0.2c

What I meant with inlining the variables was (in this case) to put them where they're first used, e.g. |for (var i = ...)|.

sr=jag

Comment 32

[Ian Neal]

(In reply to comment #31)
> (From update of attachment 213809 [details] [diff] [review] [edit])
> What I meant with inlining the variables was (in this case) to put them where
> they're first used, e.g. |for (var i = ...)|.
> 
> sr=jag
> 
Does that not cause i to be redeclared multiple times in the while loop?

Comment 33

[Ian Neal]

Attachment: Inline where applicable patch v0.2d (Checked into trunk / 1.8 branch)

Changes since v0.2c:
* Inlined variables as suggested

Carrying forward r and sr

Checking in (trunk)
extensions/wallet/signonviewer/resources/content/SignonViewer.js;
new revision: 1.11; previous revision: 1.10
xpfe/browser/resources/content/navigator.js;
new revision: 1.588; previous revision: 1.587
xpfe/components/permissions/content/cookieViewer.js;
new revision: 1.24; previous revision: 1.23
xpfe/components/permissions/content/permissionsManager.js;
new revision: 1.10; previous revision: 1.9
xpfe/components/permissions/content/permissionsManager.xul;
new revision: 1.7; previous revision: 1.6
xpfe/components/permissions/content/permissionsNavigatorOverlay.xul;
new revision: 1.32; previous revision: 1.31
xpfe/components/permissions/content/permissionsOverlay.js;
new revision: 1.18; previous revision: 1.17
xpfe/components/permissions/content/treeUtils.js;
new revision: 1.16; previous revision: 1.15
xpfe/components/permissions/locale/en-US/permissionsManager.dtd;
new revision: 1.3; previous revision: 1.2
xpfe/components/permissions/locale/en-US/permissionsManager.properties;
new revision: 1.3; previous revision: 1.2
xpfe/components/prefwindow/resources/content/pref-popups.xul;
new revision: 1.13; previous revision: 1.12
xpfe/components/prefwindow/resources/content/pref-smartupdate.xul;
new revision: 1.47; previous revision: 1.46
xpfe/components/prefwindow/resources/locale/en-US/pref-smartupdate.dtd;
new revision: 1.16; previous revision: 1.15
done

Comment 34

[Ian Neal]

Comment on attachment 213980 [details] [diff] [review]
Inline where applicable patch v0.2d (Checked into trunk / 1.8 branch)

Requesting a= for SM1.1a

Comment 35

[Robert Kaiser]

Comment on attachment 213980 [details] [diff] [review]
Inline where applicable patch v0.2d (Checked into trunk / 1.8 branch)

We badly need us to not fail silently on non-whitelisted installations, so a=me for SeaMonkey 1.1 - thanks for working on that!

Comment 36

[Stefan [:stefanh]]

Comment on attachment 213813 [details] [diff] [review]
Update to help patch v0.1

.
.
.
>+  <li><strong>Allowed Sites:</strong> Click this to view and edit the list of
>+    web sites that you want to allow to install software.

I'd prefer something like "Click this to open the Allowed Sites dialog box, where you can view and edit the list of web sites that you want to allow to install software:"

>+    <ul>
>+      <li><strong>Allowed websites</strong>: The list of allowed websites
>+        appears when you click <q>Allowed Sites</q>. You can add or remove
>+        websites that should be allowed to install software.</li>

With the above changes you wont need the explanation of the dialog box.

>+      <li><strong>Add</strong>: Click this after typing in a website that you
>+        want to add to the list.</li>

The button is called "Allow", unless you changed it when you checked in the patch... This could also be a little bit more explanatory - I think we should mention the text field somehow.

>+      <li><strong>Remove</strong>: Click this to remove a selected website.</li>

--> "Remove Site"

>+      <li><strong>Remove All</strong>: Click this to remove all of the websites

--> "Remove All Sites"

You also need to make the Help button work in the pref window ;)

Comment 37

[Ian Neal]

Attachment: Revised help patch v0.1a

Changes since help patch v0.1:
* Changes as per reviewer's comments
* Fixed dialog box help buttons to work for both software installation and image permissions

Comment 38

[Andrew Schultz]

Comment on attachment 214021 [details] [diff] [review]
Revised help patch v0.1a

>+      <li><strong>Allow</strong>: Click this after typing into the text field
>+        a web site that you want to add to the list.</li>

Was talking about this with stefanh... shouldn't the "Allow" button be "Add"?  The action we take (and the help text description) when you hit the button is to "Add" the given site to a list (of allowed sites).  And "Add" would go with the "Remove" buttons better.

Comment 39

[Stefan [:stefanh]]

Comment on attachment 214021 [details] [diff] [review]
Revised help patch v0.1a

+      <li><strong>Allow</strong>: Click this after typing into the text field
+        a web site that you want to add to the list.</li>

This was actually better before (my fault). So, r=me if you change that to "Click this to add a typed web site to the list of allowed sites."

I also think that we should remove Help buttons in dialog boxes we don't have any "real" documentation for. Hitting Help in the  "Allowed Sites" dialog box (and manage image permissions etc) now brings up the same help as when you're in the main pref window. That would be another bug, though :)

Comment 40

[Ian Neal]

Attachment: Tweaked help patch v0.1b (Checked in trunk / 1.8 branch)

Changes since v0.1a:
* Changed as per reviewer's comment.

Carrying forward r=

Checking in (trunk)
extensions/help/resources/locale/en-US/cs_nav_prefs_advanced.xhtml;
new revision: 1.44; previous revision: 1.43
xpfe/components/permissions/locale/en-US/permissionsManager.properties;
new revision: 1.4; previous revision: 1.3
done

Comment 41

[jag (Peter Annema)]

(In reply to comment #32)
> (In reply to comment #31)
> Does that not cause i to be redeclared multiple times in the while loop?

Nope, it's only declared once, even though the loop goes through it multiple times.

Comment 42

[Ian Neal]

Re-opening to fix issue for translators.

Comment 43

[Ian Neal]

Attachment: Tweaked pref-smartupdate patch v0.2e (Checked in trunk / 1.8 branch)

This patch:
* Fixes issue for localisers with longer text pushing the "Allowed Sites" button off the edge of the pref pane - take out the spacer and flex the checkbox. Tested with a longer label and wraps as it should.

Comment 44

[Ian Neal]

Comment on attachment 214850 [details] [diff] [review]
Tweaked pref-smartupdate patch v0.2e (Checked in trunk / 1.8 branch)

Checking in (trunk)
pref-smartupdate.xul;
new revision: 1.48; previous revision: 1.47
done

Comment 45

[Ian Neal]

Comment on attachment 213980 [details] [diff] [review]
Inline where applicable patch v0.2d (Checked into trunk / 1.8 branch)

Checking in (1.8 branch)
extensions/wallet/signonviewer/resources/content/SignonViewer.js;
new revision: 1.10.20.2; previous revision: 1.10.20.1
xpfe/browser/resources/content/navigator.js;
new revision: 1.577.2.18; previous revision: 1.577.2.17
xpfe/components/permissions/content/cookieViewer.js;
new revision: 1.23.8.1; previous revision: 1.23
xpfe/components/permissions/content/permissionsManager.js;
new revision: 1.9.8.2; previous revision: 1.9.8.1
xpfe/components/permissions/content/permissionsManager.xul;
new revision: 1.6.8.1; previous revision: 1.6
xpfe/components/permissions/content/permissionsNavigatorOverlay.xul;
new revision: 1.31.8.1; previous revision: 1.31
xpfe/components/permissions/content/permissionsOverlay.js;
new revision: 1.17.8.1; previous revision: 1.17
xpfe/components/permissions/content/treeUtils.js;
new revision: 1.15.8.1; previous revision: 1.15
xpfe/components/permissions/locale/en-US/permissionsManager.dtd;
new revision: 1.2.8.1; previous revision: 1.2
xpfe/components/permissions/locale/en-US/permissionsManager.properties;
new revision: 1.2.8.1; previous revision: 1.2
xpfe/components/prefwindow/resources/content/pref-popups.xul;
new revision: 1.12.6.1; previous revision: 1.12
xpfe/components/prefwindow/resources/content/pref-smartupdate.xul;
new revision: 1.46.6.1; previous revision: 1.46
xpfe/components/prefwindow/resources/locale/en-US/pref-smartupdate.dtd;
new revision: 1.13.6.3; previous revision: 1.13.6.2
extensions/help/resources/locale/en-US/cs_nav_prefs_advanced.xhtml;
new revision: 1.41.8.3; previous revision: 1.41.8.2
done