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

Tue Mar 10 19:54:33 UTC 2015

Hello,

I have attached a patch to show, what I was thinking of as a potential fix.

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)

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.

Thoughts ?

Thank you,
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: Friday, March 6, 2015 9:11:58 AM
> Subject: Re: [rfc][icedteaweb] Add comments about permission attributes not being checked in reproducers
> 
> On 03/05/2015 10:52 PM, Lukasz Dracz wrote:
> > Hello,
> >
> > ----- Original Message -----
> >> From: "Jiri Vanek" <jvanek at redhat.com>
> >> To: "Lukasz Dracz" <ldracz at redhat.com>, "IcedTea Distro List"
> >> <distro-pkg-dev at openjdk.java.net>
> >> Sent: Thursday, March 5, 2015 2:48:25 AM
> >> Subject: Re: [rfc][icedteaweb] Add comments about permission attributes
> >> not being checked in reproducers
> >>
> >> On 03/04/2015 10:00 PM, Lukasz Dracz wrote:
> >>> Hello,
> >>>
> >>> I have been looking into why certain reproducers are failing. For the
> >>> following tests
> >>>
> >>> testPartiallySignedAppletWithSandboxPermissionsInManifestLaunchWithSignedHTMLApp
> >>> testPartiallySignedJNLPAppletWithSandboxPermissionsInManifestLaunchWithSignedApp
> >>> testSignedAppletWithSandboxPermissionsInManifestHtml
> >>> testSignedAppletWithSandboxPermissionsInManifestHtmlJnlpHref
> >>> testSignedAppletWithSandboxPermissionsInManifestJnlpApplet
> >>> testSignedAppletWithSandboxPermissionsInManifestJnlpApplication
> >>>
> >>> I found that changeset 1129:0284eb954ebc is reason.
> >>>
> >>> The reason why is that the permissions attributes check has been moved
> >>> from
> >>> always being checked into an if statement
> >>> where they are only checked when deployment.manifest.attributes.check is
> >>> set to true. I have tested and found the tests to pass when this
> >>> deployment.manifest.attributes.check is set to true.
> >>>
> >>> This patch just adds comments to the tests for reference to help explain
> >>> to
> >>> anyone in the future looking at the tests. At the moment I don't have a
> >>> solution, does anyone have an idea on how to properly fix this or handle
> >>> this ?
> >>> I had tried to set the property to true for the tests and revert it back
> >>> after to no avail. I will try to think more on this problem.
> >>>
> >>
> >> Hi!
> >>
> >> So yes - the testsuite is running with
> >> deployment.manifest.attributes.check=false.
> >>
> >> IMHO best would be to tune it to run without any
> >> deployment.manifest.attributes.check.
> >>
> >>
> >> For your group of tests - I think the best way to go is :
> >> @beforeclass ensure that no deployment.manifest.attributes.check is
> >> present
> >> in
> >> deploymnet.properties. If it is, remove it.
> >> @afterclass return it back, if it was here.
> >>
> >>
> >> What do you think?  Note - tehre is plenty util methods which loads file
> >> to
> >> string or save string to
> >> file. Just do not add more :)
> >
> > I have tested the @beforeclass @afterclass method of editing the
> > deployment.properties file and well it works somewhat, some of the tests
> > pass now
> > but the other ones will prompt a pop-up and if you click to proceed it
> > passes but obviously failing to click proceed it fails.
> Maybe instead of beforeclass and after class use this setup only for
> specified tests?
> 
> >
> > This defeats the purpose of the manifest attributes check being set to
> > false.
> > I can't seem to think of a solution I would like.
> yes:(
> > I suppose we could split the deployment.manifest.attributes.check into two
> > properties, by adding a new one that decides whether to check the
> > Permissions Attribute and the default deployment.manifest.attributes.check
> > would check all the other attributes.
> >
> > For the test we would edit the manifest like you mentioned with
> > @beforeclass to change the
> > deployment.manifest.permissions.attributes.check to true and keep
> > deployment.manifest.attributes.check false.
> > What do you think about this solution ?
> > Do you have any other ideas ?
> 
> The only other idea of mine is to make deployment.manifest.attributes.check
> tri state (enum TRUE
> FALSE PERMISSIONS_ONLY) instead of boolean  an new attribute - true (all are
> tests) false (none is
> tested) all_but_permissions - then only permission s attribnute is check.
> 
> The deployment.manifest.attributes.check is itw Internal, so we can do
> moreover whatever we need
> with it.  And the code is documentation itself .
> 
> imho - the tristate is better to impl. maintain read. But instead of it, go
> with any option you wont
> to fix those tests.
> >
> > I suppose we could also try to make it possible to edit the property
> > directly and having that change updated when done. I would need to look
> > into how feasible this is.
> 
> Not sure If I understood:(
> >
> > Thoughts ?
> >
> > Thank you,
> > Lukasz Dracz
> >
> >
> >
> >
> >> Thank you!
> >>
> >> J.
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: combinationManifestAttributesCheck-2.patch
Type: text/x-patch
Size: 15901 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150310/f23e2209/combinationManifestAttributesCheck-2-0001.patch>