[rfc][icedteaweb] Add comments about permission attributes not being checked in reproducers

[Lukasz Dracz]

----- Original Message -----
> From: "Jiri Vanek" <jvanek at redhat.com>
> To: "Lukasz Dracz" <ldracz at redhat.com>
> Cc: "IcedTea Distro List" <distro-pkg-dev at openjdk.java.net>
> Sent: Tuesday, March 24, 2015 12:07:10 PM
> Subject: Re: [rfc][icedteaweb] Add comments about permission attributes not being checked in reproducers
> 
> On 03/23/2015 08:15 PM, Lukasz Dracz wrote:
> > Hello,
> >
> > I have attached the updated patch.
> >
> >>> I have attached a patch to show, what I was thinking of as a potential
> >>> fix.
> >>>
> >>
> >> maybe a bit of owerkill, but why not.
> >>
> >>> I made a new BasicValueValidator that allows combination of options
> >>> alongside single options.
> >>> I changed attributes check property from a boolean to an enum
> >>> with the following options ALL, NONE, PERMISSIONS, TRUSTED, CODEBASE,
> >>> ALAC
> >>> With the new MultipleStringValueValidator I have made it that
> >>> attributes check property can either be ALL, NONE or a
> >>> combination of PERMISSIONS, TRUSTED, CODEBASE, ALAC (delimited by "," no
> >>> spaces)
> >>
> >> Please add ENTRYPOINT
> >
> > okay
> >
> >>>
> >>> In this way it can be specified which checks you want to do
> >>> for ex. only TRUSTED and CODEBASE attributes would look like
> >>> deployment.manifest.attributes.check=TRUSTED,CODEBASE
> >>>
> >>> The fix for the tests is also included that changes the
> >>> deployment.manifest.attributes.check in the beforeClass
> >>> and puts back the previous value afterClass. Also
> >>> deployment.security.level
> >>> is changed to ALLOW_UNSIGNED and back in
> >>> the same fashion for the Partially Signed tests.
> >>>
> >>> I have mostly attached the full patch to get feedback on this direction
> >>> of
> >>> a fix ?
> >>> Any comments or thoughts would be great.
> >>>
> >>> For review I think it would be best if I split this patch into two, for
> >>> easier reviewing
> >>>
> >>> - The MultipleStringValueValidator
> >>> - The Fix with Manifest Attributes Check and in the tests
> >>>
> >>> Other option is to go with the original suggestion of just an enum with
> >>> true, false, permissions_only with the same
> >>> changing the deployment.properties BeforeClass fix. In this case, I would
> >>> prefer the enum to be ALL, NONE or
> >>> PERMISSIONS (/PERMISSIONS_ONLY) instead.
> >>
> >> if this fixs the testsm then +1 from me for what you have done here. It is
> >> allowing good versatility and actually to test what is necessary.
> >>
> >>>
> >>> Thoughts ?
> >>
> >> few inline.
> >>>
> >>>

Hello,

> +        String[] attributesCheck = deploymentProperty.split(",");
> Something duplicated here? :)

I am not sure what exactly is duplicated, if you could explain further it would be helpful.

If you mean that there is a split here, it is needed since the line above where it gets the
property it just gets the full string of the property that might contain the combination
properties separated by "," as one string.
 
> Assuming it fixed the tests, after fixing the minor above, please push.
> 
> thank you!
>

Thank you,
Lukasz Dracz