[RFC][Icedtea-Web]: Ensure all ports entered are numeric values

[Omair Majid]

On 02/28/2011 05:52 PM, Andrew Su wrote:
> Here is the updated version. I did not see the need to use
> InputVerifier for the advanced options in the network setting tab.
> Reason is because there was little to verify there which could be
> done at time of clicking "Ok". If I were to add it in there as well,
> there would be need to either make a subclass of InputVerifier to
> reduce redundant creation of anonymous classes (which was what you
> wanted to avoid[creating new class]).
>
> Comments question concerns about this patch?
>

It would be really cool if we could indicate the invalid JTextFields (or 
other control) to the users.

A few other comments in-line.

>
> diff -r 4860276e9bf2 netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsPane.java
> --- a/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsPane.java	Sun Feb 27 21:48:17 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsPane.java	Mon Feb 28 17:47:41 2011 -0500
> @@ -27,6 +27,7 @@
>   import java.awt.event.ActionListener;
>   import java.awt.event.ItemEvent;
>   import java.awt.event.ItemListener;
> +import java.text.ParseException;
>   import java.util.ArrayList;
>   import java.util.List;
>
> @@ -35,11 +36,14 @@
>   import javax.swing.JCheckBox;
>   import javax.swing.JComponent;
>   import javax.swing.JDialog;
> +import javax.swing.JFormattedTextField;
>   import javax.swing.JLabel;
> +import javax.swing.JOptionPane;
>   import javax.swing.JPanel;
>   import javax.swing.JScrollPane;
>   import javax.swing.JTextArea;
>   import javax.swing.JTextField;
> +import javax.swing.text.MaskFormatter;
>

Some of these imports look unused - I dont see any MaskFormatter or 
JFormattedTextField in this file. Please remove them.

>   import net.sourceforge.jnlp.config.DeploymentConfiguration;
>   import net.sourceforge.jnlp.runtime.Translator;
> @@ -114,28 +118,28 @@
>           // This addresses the HTTP proxy settings.
>           JLabel http = new JLabel(Translator.R("APSLabelHTTP") + ":");
>           final JTextField httpAddressField = new JTextField(fields[0]);
> -        final JTextField httpPortField = new JTextField(fields[1]);
> +        JTextField httpPortField = new JTextField(fields[1]);
>           httpAddressField.getDocument().addDocumentListener(new DocumentAdapter(fields, 0));
>           httpPortField.getDocument().addDocumentListener(new DocumentAdapter(fields, 1));
>
>           // This addresses the HTTPS proxy settings.
>           JLabel secure = new JLabel(Translator.R("APSLabelSecure") + ":");
>           final JTextField secureAddressField = new JTextField(fields[2]);
> -        final JTextField securePortField = new JTextField(fields[3]);
> +        JTextField securePortField = new JTextField(fields[3]);
>           secureAddressField.getDocument().addDocumentListener(new DocumentAdapter(fields, 2));
>           securePortField.getDocument().addDocumentListener(new DocumentAdapter(fields, 3));
>
>           // This addresses the FTP proxy settings.
>           JLabel ftp = new JLabel(Translator.R("APSLabelFTP") + ":");
>           final JTextField ftpAddressField = new JTextField(fields[4]);
> -        final JTextField ftpPortField = new JTextField(fields[5]);
> +        JTextField ftpPortField = new JTextField(fields[5]);
>           ftpAddressField.getDocument().addDocumentListener(new DocumentAdapter(fields, 4));
>           ftpPortField.getDocument().addDocumentListener(new DocumentAdapter(fields, 5));
>
>           // This addresses the Socks proxy settings.
>           JLabel socks = new JLabel(Translator.R("APSLabelSocks") + ":");
>           final JTextField socksAddressField = new JTextField(fields[6]);
> -        final JTextField socksPortField = new JTextField(fields[7]);
> +        JTextField socksPortField = new JTextField(fields[7]);
>           socksAddressField.getDocument().addDocumentListener(new DocumentAdapter(fields, 6));
>           socksPortField.getDocument().addDocumentListener(new DocumentAdapter(fields, 7));
>

Is the only change here making some final variables non-final? Is there 
a need for this?

> @@ -243,6 +247,17 @@
>           okButton.addActionListener(new ActionListener() {
>               @Override
>               public void actionPerformed(ActionEvent e) {
> +
> +                try {
> +                    if (fields[1] != null) Integer.parseInt(fields[1]);
> +                    if (fields[3] != null) Integer.parseInt(fields[3]);
> +                    if (fields[5] != null) Integer.parseInt(fields[5]);
> +                    if (fields[7] != null) Integer.parseInt(fields[7]);
> +                } catch (NumberFormatException nfe) {
> +                    JOptionPane.showMessageDialog(null, Translator.R("CPInvalidPort2"));
> +                    return;
> +                }
> +

I have a few concerns here. First, the magic constants: is there no 
other way to validate the values? Second, while you are checking that 
ports are valid integers, you may also want to check that they are valid 
port numbers. Port numbers are 16 bit integers, while Java Integers are 
32 bit integers - so it is possible to have values which is okay'ed by 
this code but fails later on.

>                   for (int i = 0; i<  fields.length; i++)
>                       config.setProperty(properties[i], fields[i]);
>
> diff -r 4860276e9bf2 netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java
> --- a/netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java	Sun Feb 27 21:48:17 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java	Mon Feb 28 17:47:41 2011 -0500
> @@ -33,9 +33,12 @@
>
>   import javax.swing.Box;
>   import javax.swing.ButtonGroup;
> +import javax.swing.InputVerifier;
>   import javax.swing.JButton;
>   import javax.swing.JCheckBox;
> +import javax.swing.JComponent;
>   import javax.swing.JLabel;
> +import javax.swing.JOptionPane;
>   import javax.swing.JPanel;
>   import javax.swing.JRadioButton;
>   import javax.swing.JTextField;
> @@ -114,6 +117,25 @@
>           addressField.getDocument().addDocumentListener(new DocumentAdapter(config, properties[1]));
>
>           final JTextField portField = new JTextField(config.getProperty(properties[2]), 3);
> +        portField.setInputVerifier(new InputVerifier() {
> +
> +            @Override
> +            public boolean verify(JComponent input) {
> +                JTextField jtf = (JTextField) input;
> +
> +                try {
> +                    String str = jtf.getText();
> +                    if (str != null&&  str.trim().length()>  0) Integer.parseInt(str);
> +                    return true;
> +                } catch (NumberFormatException nfe) {
> +                    JOptionPane.showMessageDialog(null, Translator.R("CPInvalidPort1"));
> +                    config.setProperty(properties[2], "");

Is setting the config property to "" intentional?

> +                }
> +
> +                return false;
> +            }
> +
> +        });
>           portField.getDocument().addDocumentListener(new DocumentAdapter(config, properties[2]));
>
>           // Create the button which allows setting of other types of proxy.
> diff -r 4860276e9bf2 netx/net/sourceforge/jnlp/resources/Messages.properties
> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties	Sun Feb 27 21:48:17 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties	Mon Feb 28 17:47:41 2011 -0500
> @@ -391,7 +391,9 @@
>   CVCPColName=Name
>
>   # Control Panel - Misc.
> -CPJRESupport=IcedTea-Web currently does not support the use of multiple JREs.
> +CPJRESupport=IcedTea-Web currently does not support the use of multiple JREs.
> +CPInvalidPort1=Port field entered for proxy is invalid. A port will not be specified in configuration.
> +CPInvalidPort2=There is one or more invalid port value.
>
>   # command line control panel
>   CLNoInfo=No information avaiable (is this a valid option?).

Cheers,
Omair