[RFC][PATCH][icedtea-web]: Added support for signed JNLP file- Updated Patch

[Deepak Bhole]

* Saad Mohammad <smohammad at redhat.com> [2011-07-19 11:04]:
> I have attached the updated copy of Patch1.
> 
> On 07/19/2011 02:47 AM, Jiri Vanek wrote:
> >>
> >>I hope this is the 110% for Patch1! ;)
> >99% ;)
> 
> This is for sure the 110% ;)
> 
> -- 
> Cheers,
> Saad Mohammad
> 

Hi Saad,

I have a few minor corrections for this one...

> +
> +            if (appTemplate == null && launchJNLP == null)
> +                throw new NullPointerException(
> +                        "Template JNLP file and Launching JNLP file are both null.");
> +            else if (appTemplate == null)
> +                throw new NullPointerException("Template JNLP file is null.");
> +            else if (launchJNLP == null)
> +                throw new NullPointerException("Launching JNLP file is null.");
> +

Throwing RuntimeExceptions is generally not a good idea. Consider making
the above a checked exception.

> +            XMLElement appTemplateXML = new XMLElement();
> +            XMLElement launchJNLPXML = new XMLElement();
> +
> +            // Remove the comments and CDATA from the JNLP file
> +            final PipedInputStream pinTemplate = new PipedInputStream();
> +            final PipedOutputStream poutTemplate = new PipedOutputStream(pinTemplate);
> +            appTemplateXML.sanitizeInput(appTemplate, poutTemplate);
> +
> +            final PipedInputStream pinJNLPFile = new PipedInputStream();
> +            final PipedOutputStream poutJNLPFile = new PipedOutputStream(pinJNLPFile);
> +            launchJNLPXML.sanitizeInput(launchJNLP, poutJNLPFile);
> +

The above streams are never closed.

> +            // Parse both files
> +            appTemplateXML.parseFromReader(new InputStreamReader(pinTemplate));
> +            launchJNLPXML.parseFromReader(new InputStreamReader(pinJNLPFile));
> +
> +            // Initialize parent nodes
> +            this.appTemplateNode = new Node(appTemplateXML);
> +            this.launchJNLPNode = new Node(launchJNLPXML);
> +            this.isTemplate = isTemplate;
> +
> +        } catch (Exception e) {
> +            throw new JNLPMatcherException(
> +                    "Failed to create an instance of JNLPVerify with specified InputStreamReader",
> +                    e);
> +        }
> +    }
> +
> +    /**
> +     * Compares both JNLP files
> +     * 
> +     * @return true if both JNLP files are 'matched', otherwise false
> +     */
> +    public boolean isMatch() {
> +
> +        if (match == null)
> +            match = matchNodes(appTemplateNode, launchJNLPNode);
> +        
> +        return match;
> +    }
> +

Is there a case where match needs to be cached (i.e. isMatch is called
multiple times for the same JNLPMatcher)? If not, the above function is
not needed.

...
> +
> +    /**
> +     * Getter for application/template Node
> +     * 
> +     * @return the Node of the signed application/template file
> +     */
> +    public Node getAppTemplateNode() {
> +        return appTemplateNode;
> +    }
> +
> +    /**
> +     * Getter for launching application Node
> +     * 
> +     * @return the Node of the launching JNLP file
> +     */
> +    public Node getLaunchJNLPNode() {
> +        return launchJNLPNode;
> +    }
> +

I don't see anything calling the above .. the functions should be
removed in that case.

...
> @@ -86,6 +102,7 @@
>      private ParsedXML tinyNode;
>      private Node next;
>      private Node children[];
> +    private String attributeNames[];
>  
>      Node(ParsedXML tinyNode) {
>          this.tinyNode = tinyNode;
> @@ -127,6 +144,19 @@
>  
>          return children;
>      }
> +    
> +    String[] getAttributeNames() {
> +        if (attributeNames == null) {
> +            List<String> list = new ArrayList<String>();
> +
> +            for (Enumeration e = xml.enumerateAttributeNames(); e.hasMoreElements();)
> +                list.add(new String((String) e.nextElement()));
> +
> +            attributeNames = list.toArray(new String[list.size()]);
> +
> +        }
> +        return attributeNames;
> +    }
>  

Isn't the above all in commented code? Line 100 to 176 is commented out
in HEAD:
http://icedtea.classpath.org/hg/icedtea-web/file/7dd63058c234/netx/net/sourceforge/jnlp/Node.java

I will leave all of the test case reviews to Jiri :)

Cheers,
Deepak