[rfc][icedteaweb] support for entry-point manifest atribute

[Jiri Vanek]

On 03/03/2015 05:20 PM, Jie Kang wrote:
>
>
> ----- Original Message -----
>> On 02/25/2015 06:19 PM, Jiri Vanek wrote:
>>> Hello, this is impl of another security manifest att:
>>> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#entry_pt
>>> I would like to go with it to 1.5 too.
>>>
>>> Speaking about the attributes:
>>> does http://www.geogebra.org/webstart/geogebra.jnlp library loading promt
>>> seems correct to you?
>>>
>>> J.
>> Crap - improved one
>>
>> The check is valid only if RIA is signed. I had it removed for testing...
>
> Hello,
>
>
> Nice patch :D
>
> It might be nice for 1.5 but isn't 1.5 released already? We shouldn't really add new functionality, especially something like this which could easily break user experience.
>
> Ie. apps that worked with 1.5.(x) don't work with 1.5.(x+1) anymore (even though it is correct).

Agree.
>
>
> +    static String[] splitEntryPoints(String eps) {
>
> The use of protected static here seems to be only so it's easier to test, yes? Please add a comment saying something like //static and package-private for testing

sure.
>
> Also, s/eps/entryPointString or something like that. No need to shorten it to something unreadable nowadays.

also done.
>
>
> +        throw new LaunchException("None of '" + file.getManifestsAttributes().getEntryPointString() + "' matched yours " + mainClass + ". Thats fatal.");
>
>
> It's not clear to me what exactly is happening with the current message.
>
> Can you change it to something similar to:
>
>
> "None of the entry points matched the main class and the applet is signed. This is a security error and the applet will not be launched."

Used your way (with params like in mine original)
>
>
>
> +        @Test
>
> Please remove the extra tab.
>
> +    public void splitEntryPointsReturnsTests() throws Exception {
> +        Assert.assertArrayEquals(null, JNLPFile.splitEntryPoints("  "));
> +        Assert.assertArrayEquals(null, JNLPFile.splitEntryPoints(null));
> +        Assert.assertArrayEquals(new String[]{"a.b.c"}, JNLPFile.splitEntryPoints("  a.b.c  "));
> +        Assert.assertArrayEquals(new String[]{"a.b.c"}, JNLPFile.splitEntryPoints("a.b.c"));
> +        Assert.assertArrayEquals(new String[]{"a.b.c"}, JNLPFile.splitEntryPoints("  a.b.c"));
> +        Assert.assertArrayEquals(new String[]{"a.b.c"}, JNLPFile.splitEntryPoints("a.b.c  "));
> +        Assert.assertArrayEquals(new String[]{"a.b.c", "cde"}, JNLPFile.splitEntryPoints("  a.b.c     cde"));
> +        Assert.assertArrayEquals(new String[]{"a.b.c", "cde"}, JNLPFile.splitEntryPoints("  a.b.c cde    "));
> +        Assert.assertArrayEquals(new String[]{"a.b.c", "cde"}, JNLPFile.splitEntryPoints("a.b.c         cde    "));
>
> As easy as it is, it's bad style to put all your tests in one place. Readers can't as easily tell what cases you're testing when you do this. You generally want to minimize a test to a single assert if possible, and split them into multiple test methods that can be descriptively named.
>
> Can you put them in separate tests with appropriate names?
>
> +        Assert.assertNull("classlaoder attached, but should be null", jnlpFile.getManifestsAttributes().getAttribute(new Attributes.Name(JNLPFile.ManifestsAttributes.ENTRY_POINT)));

aarggg.... Ok. Split to logical group of three tests methods.
>
> While you're here can you
>
> s/classlaoder/classloader

also done. Thanx.
>
> Not really required but it looks nicer with correct spelling;;
>
>
> Rest looks great!
>
>

Thanx pushed.



As for issues jacob higlighted.
  - no checking for fully classified  classname
  - traversing all jars

Especially the second may really be dangerous.  For first, I really dont know how it may be threating.

But both should be revisited, especially the second one and especially because it is affecting all checked attributes.


J.