[rfc][icedteaweb] Add comments about permission attributes not being checked in reproducers
Lukasz Dracz
ldracz at redhat.com
Mon Mar 23 19:15:48 UTC 2015
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.
> >
> >
> > 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.
> >>>> > >>
> >>>> > >>
> >>>> > >>
> >>>> > >>
> >>>> > >>
> >>>> > >>
> >>>> > >>
> >> >
> >> >
> >
> > combinationManifestAttributesCheck-2.patch
> >
> >
> > diff --git a/netx/net/sourceforge/jnlp/config/BasicValueValidators.java
> > b/netx/net/sourceforge/jnlp/config/BasicValueValidators.java
> > --- a/netx/net/sourceforge/jnlp/config/BasicValueValidators.java
> > +++ b/netx/net/sourceforge/jnlp/config/BasicValueValidators.java
> > @@ -202,6 +202,69 @@
> > }
> >
> > /**
> > + * Checks that the value is one of the acceptable single String values
> > + * or an acceptable combination of String values
> > + */
> > + private static class MultipleStringValueValidator implements
> > ValueValidator {
> > + String[] singleOptions = null;
> > + String[] comboOptions = null;
>
> make them final.
>
> They may be set in constructor only.
Okay
> > +
> > + public MultipleStringValueValidator(String[] singleOptions,
> > String[] comboOptions) {
> > + this.singleOptions = singleOptions;
> > + this.comboOptions = comboOptions;
> > + }
> > +
> > + @Override
> > + public void validate(Object value) throws IllegalArgumentException
> > {
> > + Object possibleValue = value;
> > + if (!(possibleValue instanceof String)) {
> > + throw new IllegalArgumentException("Must be a string");
> > + }
> > +
> > + String stringVal = (String) possibleValue;
> > + boolean found = false;
> > + for (String knownVal : singleOptions) {
> > + if (knownVal.equals(stringVal)) {
> > + found = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!found) {
> > + String[] possibleCombo = stringVal.split(",");
>
> separate this split in into static method in this class. and move the "," to
> constant.
okay
> See the reasons for this later in code
>
> > + for (String val : possibleCombo) {
> > + if (comboOptionsContains(val)) {
> > + found = true;
> > + } else {
> > + throw new IllegalArgumentException();
> > + }
> > + }
> > + }
> > +
> > + if (!found) {
> > + throw new IllegalArgumentException();
> > + }
> > + }
> > +
> > + private boolean comboOptionsContains(String possibleVal) {
> > + for (String value : comboOptions) {
> > + if (value.equals(possibleVal)) {
> > + return true;
> > + }
> > + }
> > + return false;
> > + }
> > +
> > + @Override
> > + public String getPossibleValues() {
> > + String message = "(Values that can be used alone only): " +
> > Arrays.toString(singleOptions) +
> > + " (Values that can be used in combination): " +
> > Arrays.toString(comboOptions);
> Mention in second one that the constant of above is an delimiter. and no
> space expected
> Also it can behave nicer if one of the arrays is null or empty...
>
> > + return message;
> > + }
>
> Except this one, use also
>
> ManifestAttributeCheckValidator extends MultipleStringValueValidator
>
> public ManifestAttributeCheckValidator(){
> super(youKNowTheValues1,youKNowTheValues2)
>
> Check the behaviour with
> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2015-March/031043.html
okay
> }
> > +
> > + }
> > +
> > + /**
> > * Checks that the value is a URL
> > */
> > private static class UrlValidator implements ValueValidator {
> > @@ -262,6 +325,18 @@
> > }
> >
> > /**
> > + * Returns a {@link ValueValidator} that checks if an object is a
> > string from
> > + * one of the provided single option Strings or a combination from
> > + * the provided combination Strings.
> > + * @param singleValues an array of Strings which are considered valid
> > only by themselves
> > + * @param comboValues an array of Strings which are considered valid
> > in any combination
> > + * with themselves
> > + */
> > + public static ValueValidator getMultipleStringValidator(String[]
> > singleValues, String[] comboValues) {
> > + return new MultipleStringValueValidator(singleValues,
> > comboValues);
> > + }
>
>
> add getter also for the ManifestAttributeCheckValidator
sure
>
>
> > +
> > + /**
> > * @return a {@link ValueValidator} that checks if an object
> > represents a
> > * valid url
> > */
> > diff --git a/netx/net/sourceforge/jnlp/config/Defaults.java
> > b/netx/net/sourceforge/jnlp/config/Defaults.java
> > --- a/netx/net/sourceforge/jnlp/config/Defaults.java
> > +++ b/netx/net/sourceforge/jnlp/config/Defaults.java
> > @@ -43,6 +43,8 @@
> > import net.sourceforge.jnlp.ShortcutDesc;
> > import static net.sourceforge.jnlp.config.PathsAndFiles.*;
> > import net.sourceforge.jnlp.runtime.JNLPProxySelector;
> > +import net.sourceforge.jnlp.runtime.ManifestAttributesChecker;
> > +
> > import static net.sourceforge.jnlp.runtime.Translator.R;
> >
> > /**
> > @@ -409,8 +411,16 @@
> > //enable manifest-attributes checks
> > {
> > DeploymentConfiguration.KEY_ENABLE_MANIFEST_ATTRIBUTES_CHECK,
> > - BasicValueValidators.getBooleanValidator(),
> > - String.valueOf(true)
> > +
> > BasicValueValidators.getMultipleStringValidator(new
> > String[] {
> > +
> > ManifestAttributesChecker.MANIFEST_ATTRIBUTES_CHECK.ALL.toString(),
> > +
> > ManifestAttributesChecker.MANIFEST_ATTRIBUTES_CHECK.NONE.toString()
> > + }, new String[] {
> > +
> > ManifestAttributesChecker.MANIFEST_ATTRIBUTES_CHECK.PERMISSIONS.toString(),
> > +
> > ManifestAttributesChecker.MANIFEST_ATTRIBUTES_CHECK.CODEBASE.toString(),
> > +
> > ManifestAttributesChecker.MANIFEST_ATTRIBUTES_CHECK.TRUSTED.toString(),
> > +
> > ManifestAttributesChecker.MANIFEST_ATTRIBUTES_CHECK.ALAC.toString()
>
> USe the ManifestAttributeCheckValidator
> > + }),
> > + String.valueOf("ALL")
> > }
> > };
> >
> > diff --git
> > a/netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java
> > b/netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java
> > --- a/netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java
> > +++ b/netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java
> > @@ -37,7 +37,9 @@
> > package net.sourceforge.jnlp.runtime;
> >
> > import java.net.URL;
> > +import java.util.Arrays;
> > import java.util.HashSet;
> > +import java.util.List;
> > import java.util.Set;
> >
> > import net.sourceforge.jnlp.ExtensionDesc;
> > @@ -75,21 +77,45 @@
> > this.securityDelegate = securityDelegate;
> > }
> >
> > + public enum MANIFEST_ATTRIBUTES_CHECK {
> > + ALL,
> > + NONE,
> > + PERMISSIONS,
> > + CODEBASE,
> > + TRUSTED,
> > + ALAC
>
> Please add also ENTRYPOINT (or I will forget it in next patch)
Yes
> > + }
> > +
> > void checkAll() throws LaunchException {
> > - if (isCheckEnabled()) {
> > - checkPermissionsAttribute();
> > + List<String> attributesCheck = getAttributesCheck();
> > + if
> > (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.ALL.toString())) {
> > checkTrustedOnlyAttribute();
> > checkCodebaseAttribute();
> > checkPermissionsAttribute();
> > checkApplicationLibraryAllowableCodebaseAttribute();
> > } else {
> > -
> > OutputController.getLogger().log(OutputController.Level.WARNING_ALL,
> > MANIFEST_CHECK_DISABLED_MESSAGE);
> > + if
> > (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.TRUSTED.toString())) {
> > + checkTrustedOnlyAttribute();
> > + }
> > + if
> > (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.CODEBASE.toString()))
> > {
> > + checkCodebaseAttribute();
> > + }
> > + if
> > (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.PERMISSIONS.toString()))
> > {
> > + checkPermissionsAttribute();
> > + }
> > + if
> > (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.ALAC.toString())) {
> > + checkApplicationLibraryAllowableCodebaseAttribute();
> > + }
> > + if
> > (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.NONE.toString())) {
> > +
> > OutputController.getLogger().log(OutputController.Level.WARNING_ALL,
> > MANIFEST_CHECK_DISABLED_MESSAGE);
> > + }
>
>
> I would rewrite this hunk a bit:
>
> List<String> attributesCheck = getAttributesCheck();
> if
> (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.ALL.toString()))
> {
> checkTrustedOnlyAttribute();
> checkCodebaseAttribute();
> checkPermissionsAttribute();
> checkApplicationLibraryAllowableCodebaseAttribute();
> } else {
> if
> (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.TRUSTED.toString()))
> {
> checkTrustedOnlyAttribute();
> }
> if
> (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.CODEBASE.toString()))
> {
> checkCodebaseAttribute();
> }
> if
> (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.PERMISSIONS.toString()))
> {
> checkPermissionsAttribute();
> }
> if
> (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.ALAC.toString()))
> {
> checkApplicationLibraryAllowableCodebaseAttribute();
> }
> if
> (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.NONE.toString()))
> {
> OutputController.getLogger().log(OutputController.Level.WARNING_ALL,
> MANIFEST_CHECK_DISABLED_MESSAGE);
> }
> }
>
> =>
>
> List<String> attributesCheck = getAttributesCheck();
> *maybe this can be done on enum level rather then on Sting level?
>
> if (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.TRUSTED)
> OR attributesCheck.haveOnlyAll) {
> checkTrustedOnlyAttribute();
> } else {
> OutputController.getLogger().log(OutputController.Level.WARNING_ALL,
> "check on Trusted-Only skipped because properrty of
> enable.manifest... have value(s) of: reprint that);
> if
> (attributesCheck.contains(MANIFEST_ATTRIBUTES_CHECK.CODEBASE.toString()))
> {
> checkCodebaseAttribute();
> }
> ... same for rest. What do you think?
I rewrote it, I think it follows what you outlined.
> }
>
> > }
> > }
> >
> > - public static boolean isCheckEnabled() {
> > + public static List<String> getAttributesCheck() {
> > final String deploymentProperty =
> > JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_ENABLE_MANIFEST_ATTRIBUTES_CHECK);
> > - return Boolean.parseBoolean(deploymentProperty);
> > + String[] attributesCheck = deploymentProperty.split(",");
> > + return Arrays.asList(attributesCheck);
>
> Well, map back to ENUM rather.
Okay
> > }
> >
> > /**
> > diff --git a/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPFileTest.java
> > b/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPFileTest.java
> > --- a/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPFileTest.java
> > +++ b/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPFileTest.java
> > @@ -65,7 +65,7 @@
> > @BeforeClass
> > public static void setPermissions() {
> > level =
> > AppletStartupSecuritySettings.getInstance().getSecurityLevel();
> > - attCheckValue = ManifestAttributesChecker.isCheckEnabled();
> > + attCheckValue = ManifestAttributesChecker.getAttributesCheck();
> > JNLPRuntime.getConfiguration().setProperty(DeploymentConfiguration.KEY_SECURITY_LEVEL,
> > AppletSecurityLevel.ALLOW_UNSIGNED.toChars());
> > JNLPRuntime.getConfiguration().setProperty(DeploymentConfiguration.KEY_ENABLE_MANIFEST_ATTRIBUTES_CHECK,
> > String.valueOf(true));
>
> Are you sure that this is the only change needed here?
>
> JNLPRuntime.getConfiguration().setProperty(DeploymentConfiguration.KEY_ENABLE_MANIFEST_ATTRIBUTES_CHECK,
> String.valueOf(attCheckValue));
>
> sounds more fragile...
Ahh I used the IDE switch method name every occurrence but I didn't look at the occurrences to see if more changes where needed.
I have fixed that, good catch !
> > }
>
> Please add one or two simple unittests for new (two!) validators
> > diff --git
> > a/tests/reproducers/custom/PartiallySignedAppletManifestSpecifiesSandbox/testcases/PartiallySignedAppletManifestSpecifiesSandboxTests.java
> > b/tests/reproducers/custom/PartiallySignedAppletManifestSpecifiesSandbox/testcases/PartiallySignedAppletManifestSpecifiesSandboxTests.java
> > ---
> > a/tests/reproducers/custom/PartiallySignedAppletManifestSpecifiesSandbox/testcases/PartiallySignedAppletManifestSpecifiesSandboxTests.java
> > +++
> > b/tests/reproducers/custom/PartiallySignedAppletManifestSpecifiesSandbox/testcases/PartiallySignedAppletManifestSpecifiesSandboxTests.java
> > @@ -36,6 +36,10 @@
> > */
> >
> > import static org.junit.Assert.assertTrue;
> > +
> > +import java.io.File;
> > +import java.io.IOException;
> > +
> > import net.sourceforge.jnlp.ProcessResult;
> > import net.sourceforge.jnlp.annotations.Bug;
> > import net.sourceforge.jnlp.annotations.NeedsDisplay;
> > @@ -44,6 +48,9 @@
> > import net.sourceforge.jnlp.browsertesting.Browsers;
> > import net.sourceforge.jnlp.closinglisteners.AutoOkClosingListener;
> >
> > +import net.sourceforge.jnlp.util.FileUtils;
> > +import org.junit.AfterClass;
> > +import org.junit.BeforeClass;
> > import org.junit.Test;
> >
> > public class PartiallySignedAppletManifestSpecifiesSandboxTests extends
> > BrowserTest {
> > @@ -56,6 +63,67 @@
> > private static final String STACKTRACE_NOT_GRANT_PERMISSIONS_TYPE =
> > "Cannot grant permissions to unsigned jars";
> > private static final String USER_HOME =
> > System.getProperty("user.home");
> >
> > + private static File deployFile;
> > + private static String attributesCheck;
> > + private static String securityLevel;
> > + private static boolean foundAttributesCheck = false;
> > + private static boolean foundSecurityLevel = false;
> > +
> > + @BeforeClass
> > + public static void setupDeploymentProperties() throws IOException {
> > + String location = System.getProperty("user.home") +
> > "/.config/icedtea-web/deployment.properties";
>
> NO! use PathsAndFiles!!!
>
> > + deployFile = new File(location);
> > + String properties = FileUtils.loadFileAsString(deployFile);
> > + String deployProperties = "";
> > +
>
> Please simplify this:
Yes, I have simplified it and I used PathsAndFiles now.
Sorry wasn't aware of PathsAndFiles.
>
>
> > + for (String line : properties.split("\n")) {
> > + if (line.contains("deployment.manifest.attributes.check")) {
> > + attributesCheck = line;
> > + deployProperties +=
> > "deployment.manifest.attributes.check=PERMISSIONS\n";
> > + foundAttributesCheck = true;
>
> not needed, or not? You can use attributesCheck and null/not null
> > + } else if (line.contains("deployment.security.level")) {
> > + securityLevel = line;
> > + deployProperties +=
> > "deployment.security.level=ALLOW_UNSIGNED\n";
> > + foundSecurityLevel = true;
> same here, or not?
> > + } else {
> > + deployProperties += line + "\n";
> > + }
> > + }
> > +
> > + if (!foundAttributesCheck) {
> > + deployProperties +=
> > "deployment.manifest.attributes.check=PERMISSIONS\n";
> > + }
> > + if (!foundSecurityLevel) {
> > + deployProperties +=
> > "deployment.security.level=ALLOW_UNSIGNED\n";
> > + }
> > +
> > + FileUtils.saveFile(deployProperties, deployFile);
> > + }
> > +
> > + @AfterClass
> > + public static void setbackDeploymentProperties() throws IOException {
> > + String location = System.getProperty("user.home") +
> > "/.config/icedtea-web/deployment.properties";
>
> NO! use PathsAndFiles!!!
>
> > + deployFile = new File(location);
> > + String properties = FileUtils.loadFileAsString(deployFile);
> > + String deployProperties = "";
> > +
> > + for (String line : properties.split("\n")) {
> > + if (line.contains("deployment.manifest.attributes.check")) {
> > + if (foundAttributesCheck) {
> > + deployProperties += attributesCheck + "\n";
> > + }
> > + } else if (line.contains("deployment.security.level")) {
> > + if (foundSecurityLevel) {
> > + deployProperties += securityLevel + "\n";
> > + }
> > + } else {
> > + deployProperties += line + "\n";
> > + }
> > + }
> > +
> > + FileUtils.saveFile(deployProperties, deployFile);
> > + }
> > +
> > @Test
> > @NeedsDisplay
> > @TestInBrowsers(testIn={Browsers.one})
> > diff --git
> > a/tests/reproducers/signed/SignedAppletManifestSpecifiesSandbox/testcases/SignedAppletManifestSpecifiesSandboxTests.java
> > b/tests/reproducers/signed/SignedAppletManifestSpecifiesSandbox/testcases/SignedAppletManifestSpecifiesSandboxTests.java
> > ---
> > a/tests/reproducers/signed/SignedAppletManifestSpecifiesSandbox/testcases/SignedAppletManifestSpecifiesSandboxTests.java
> > +++
> > b/tests/reproducers/signed/SignedAppletManifestSpecifiesSandbox/testcases/SignedAppletManifestSpecifiesSandboxTests.java
> > @@ -36,6 +36,10 @@
> > */
> >
> > import static org.junit.Assert.assertTrue;
> > +
> > +import java.io.File;
> > +import java.io.IOException;
> > +
> > import net.sourceforge.jnlp.ProcessResult;
> > import net.sourceforge.jnlp.annotations.Bug;
> > import net.sourceforge.jnlp.annotations.NeedsDisplay;
> > @@ -44,6 +48,9 @@
> > import net.sourceforge.jnlp.browsertesting.Browsers;
> > import net.sourceforge.jnlp.closinglisteners.AutoOkClosingListener;
> >
> > +import net.sourceforge.jnlp.util.FileUtils;
> > +import org.junit.AfterClass;
> > +import org.junit.BeforeClass;
> > import org.junit.Test;
> >
> > public class SignedAppletManifestSpecifiesSandboxTests extends
> > BrowserTest {
> > @@ -56,6 +63,54 @@
> > private static final String JNLP_EXPECTED_STDOUT = "Initialization
> > Error";
> > private static final String JNLP_EXPECTED_STDERR =
> > "net.sourceforge.jnlp.LaunchException";
> >
> > + private static File deployFile;
> > + private static String attributesCheck;
> > + private static boolean foundAttributesCheck = false;
> > +
> > + @BeforeClass
> > + public static void setupDeploymentProperties() throws IOException {
> > + String location = System.getProperty("user.home") +
> > "/.config/icedtea-web/deployment.properties";
> > + deployFile = new File(location);
> > + String properties = FileUtils.loadFileAsString(deployFile);
> > + String deployProperties = "";
> > +
> > + for (String line : properties.split("\n")) {
> > + if (line.contains("deployment.manifest.attributes.check")) {
> > + attributesCheck = line;
> > + deployProperties +=
> > "deployment.manifest.attributes.check=PERMISSIONS\n";
> > + foundAttributesCheck = true;
> > + } else {
> > + deployProperties += line + "\n";
> > + }
> > + }
> > +
> > + if (!foundAttributesCheck) {
> > + deployProperties +=
> > "deployment.manifest.attributes.check=PERMISSIONS\n";
> > + }
> > +
> > + FileUtils.saveFile(deployProperties, deployFile);
> > + }
> > +
> > + @AfterClass
> > + public static void setbackDeploymentProperties() throws IOException {
> > + String location = System.getProperty("user.home") +
> > "/.config/icedtea-web/deployment.properties";
> > + deployFile = new File(location);
> > + String properties = FileUtils.loadFileAsString(deployFile);
> > + String deployProperties = "";
> > +
> > + for (String line : properties.split("\n")) {
> > + if (line.contains("deployment.manifest.attributes.check")) {
> > + if (foundAttributesCheck) {
> > + deployProperties += attributesCheck + "\n";
> > + }
> > + } else {
> > + deployProperties += line + "\n";
> > + }
> > + }
> > +
> > + FileUtils.saveFile(deployProperties, deployFile);
> > + }
> > +
> > @Test
> > @NeedsDisplay
> > @TestInBrowsers(testIn={Browsers.one})
> >
>
> Great work!
>
> J.
>
Thank you for the review !
Regards,
Lukasz Dracz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: combinationManifestAttributesCheck-3.patch
Type: text/x-patch
Size: 25692 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150323/e14ceae7/combinationManifestAttributesCheck-3-0001.patch>
More information about the distro-pkg-dev
mailing list