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

Tue Jan 14 04:42:46 PST 2014

On 01/14/2014 12:33 AM, Jacob Wisor wrote:
> 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! :-)

Yes this would be :))
But it is different task. And Quite complex. Especially it must pass upstream (openjdk). And that is 
*the* task!

For now I'm happy that this feature was implemented with such an small effort.
>
> [...]
>
> 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. ;-)
>

hehehe, nice :)

> +            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! :-)
>

Except Jacob's hints I have (partially shared with him) few thoughts:

+                launchPolicyTool(config.getProperty(DeploymentConfiguration.KEY_USER_SECURITY_POLICY));

The file which is used by polici tool (and so by itw settings, and especially later, by ITW in 
runtime, should be shown in ITW seetings (it a must for current patch).
Is it worthy to have more then one policy file? If so, then this file should editable(swithcable) by 
itw settings.
Does it have sense to have some mayor file (in etc/jvm) dir?

+            final String[] policyToolCmd = new String[] { "policytool", "-file", 
policyFile.getCanonicalPath() };

This scares me. javaws and itw-settings are supposed to be windows compatible at least somehow. I'm 
not sure how this will pass.
I would vote rather for adding (tools.jar?) items (if necessary) to (boot)classpath, and  then 
invoke main method (as policitool binary file does). Also it will allow you to have more control 
over awt thread as Jacob suggested.

So - I suppose that *usage* of this file is already implemented in itw? If so (and I guess so :), 
the reproducers will be needed.
At least two - one allowing some permissions, one to removing some permissions. It is also an must 
for the patch :(

Otherwise - excellent improvement to itw!
Thanx a lot for it!
   J.