[rfc][icedtea-web] policytool in itweb-settings

Mon Jan 13 15:33:02 PST 2014

Hello there!

On 01/13/2014 23:20, Andrew Azores wrote:
> Hi,
>
> This small patch hooks the JDK policytool into itweb-settings. It can then be
> used to set up a custom user-level JNLP policy - this, in combination with the
> Run in Sandbox patch, will allow for quite a lot more flexibility in how
> permissions are handled with signed applets/applications.
>
> A nicer, more user-friendly editor to replace the policytool will hopefully come
> later on.

Oooooooh yes, please! This would be awesome! :-)

[...]

diff --git a/netx/net/sourceforge/jnlp/controlpanel/PolicyPanel.java 
b/netx/net/sourceforge/jnlp/controlpanel/PolicyPanel.java
new file mode 100644
--- /dev/null
+++ b/netx/net/sourceforge/jnlp/controlpanel/PolicyPanel.java
[...]
+import net.sourceforge.jnlp.config.DeploymentConfiguration;
+import net.sourceforge.jnlp.util.logging.OutputController;
+
+public class PolicyPanel extends NamedBorderPanel {

Is it conceivable that this PolicyPanel is going to be extended or is supposed 
to be reusable API code? If not, you may consider marking it final.

+
+    private final DeploymentConfiguration config;
+
+    public PolicyPanel(final DeploymentConfiguration config) {

Perhaps the constructor's access modifier set to default could be a better 
choice? After all, this PolicyPanel is not supposed to be used outside of its 
package. I have not looked myself, but if the other panel's constructors in this 
package also have their access modifiers set to public, it should be fine.

+        super(R("CPHeadPolicy"), new GridBagLayout());
+        this.config = config;
+
+        addComponents();
+    }
+
+    private void addComponents() {
+        JLabel aboutLabel = new JLabel("<html>" + R("CPPolicyDetail") + "</html>");
+        JButton showUserPolicyButton = new JButton(R("CPButPolicy"));
+        showUserPolicyButton.addActionListener(new ActionListener() {

Well, it is not wrong, but I am really not a fan of anonymous classes in UI 
code. There are probably better situations to use this language construct but 
unfortunately hundreds of examples in Java programming books have been printed 
with those rather bluntly authored examples. Hence thousands of students have 
taken over this habit not knowing any better that the author of that book was 
just either too lazy to type a real world example, had to fit it on one page, or 
just wanted to make stuff clear with compact code. Or, maybe the author just 
wanted to say: Hey look! Java is so cool! Look how few characters you need to 
type to get visible effects? I do not know. :-/ My point is that often this kind 
of code gets difficult to read and the class looses structure over time, 
especially after it has been edited by multiple people over the years. So, it 
pays off to name things and let classes have structure from the start on. ;-)

+            public void actionPerformed(ActionEvent e) {
+ 
launchPolicyTool(config.getProperty(DeploymentConfiguration.KEY_USER_SECURITY_POLICY));

Hmm,... grumble...grumble... Do you really want to try to exec a new process on 
the AWT thread? A thousand things could go wrong while blocking it. Just one 
example might be network access.

+            }
+        });
+        GridBagConstraints c = new GridBagConstraints();
+        c.fill = GridBagConstraints.BOTH;
+        c.gridx = 1;
+        c.gridy = 0;
+        c.weightx = 1;
+        add(aboutLabel, c);
+        c.fill = GridBagConstraints.NONE;
+        c.weighty = 0;
+        c.weightx = 0;
+        c.gridx = 1;
+        c.gridy++;
+        add(showUserPolicyButton, c);
+        /* Keep all the elements at the top of the panel (Extra padding) */
+        c.fill = GridBagConstraints.BOTH;
+        Component filler = Box.createRigidArea(new Dimension(1, 1));
+        c.weighty = 1;
+        c.gridy++;
+        add(filler, c);
+    }
+
+    private static void launchPolicyTool(String fileUrl) {
+        try {
+            final URL url = new URL(fileUrl);
+            final File policyFile = new File(url.getPath());
+            final String[] policyToolCmd = new String[] { "policytool", 
"-file", policyFile.getCanonicalPath() };
+            Runtime.getRuntime().exec(policyToolCmd);
+        } catch (IOException e) {
+            OutputController.getLogger().log(e);
+        }
+    }

As already mentioned above, this should probably better not block the AWT thread.

Otherwise looks okay to start with, as long as you keep up (your promise?) to 
effectively replace OpenJDK's policytool with something more accessible and 
convenient. Keep it up! :-)

Jacob