[rfc][icedtea-web] fix CodeBaseManifestEntrySignedMatching Tests by enabling Codebase check

Jiri Vanek jvanek at redhat.com
Fri Apr 3 14:50:42 UTC 2015


Hi lukas!


I'm poushing this for you ( I'm starting tests rightnow, and I'm really asked for numbers:( )

I will fix the hardcoded values.

Also I noted one bug - when you used two  modifiers in sequence, you restored them wrongly:

+        permissionsModifier = new DeploymentPropetiesModifier();
+        permissionsModifier.setProperties(deployment.manifest.attributes.check, PERMISSIONS);
+        securityLevelModifier = new DeploymentPropetiesModifier();
+        securityLevelModifier.setProperties(deployment.security.level, ALLOW_UNSIGNED);

and restore
+        permissionsModifier.restoreProperties();
+        securityLevelModifier.restoreProperties();


afaik it shouldbe:
+        securityLevelModifier.restoreProperties();
+        permissionsModifier.restoreProperties();

or actually only
  permissionsModifier.restoreProperties();

(as they rember first satte).
But not defintley the one you have used.
(5minutes laer - the order probably does not metter.. sorry!)

Please correct me if I'm wrong!

Maybe you can enhance your impl to take more params?
Or add gathering impl? But its probably completly wasting of time :)


Please add tests for this utility method in some reasonable time.


tyvm! and sorry for my push.
J.
On 04/02/2015 07:41 PM, Jiri Vanek wrote:
> On 04/02/2015 05:53 PM, Lukasz Dracz wrote:
>> Hello,
>>
>>>> > >I have been looking into why these tests fail:
>>>> > >
>>>> > >- ApplicationJNLPLocalTest
>>>> > >- AppletJNLPRLocalTest
>>>> > >- ApplicationJNLPRemoteTest
>>>> > >- BrowserAppletLocalTest
>>>> > >- BrowserJNLPHrefRemoteTest
>>>> > >- BrowserJNLPHrefLocalTest
>>>> > >- AppletJNLPRemoteTest
>>>> > >- BrowserAppletRemoteTest
>>>> > >- ApplicationJNLPLocalTestWithRemoteCodebase
>>>> > >
>>>> > >I have found that the changeset 984 where manifest attributes check
>>>> > >property was introduced to be the problem. The tests pass regularly if the
>>>> > >check is enabled to ALL or contains CODEBASE in the combination of
>>>> > >attributes check.
>>> >
>>> >Thanx for investigations! Why they need the attributes at all?
>>> >
>>>> > >But since when the reproducers are typically run the attributes check flag
>>>> > >is set to NONE, I have made this patch that adds a BeforeClass and
>>>> > >AfterClass that changes the manifest attributes check property to CODEBASE
>>>> > >and back to its previous value after the tests. This ensures the tests
>>>> > >pass no matter what the deployment.manifest.attributes.check is set to.
>>>> > >
>>>> > >Thank you,
>>>> > >Lukasz Dracz
>>>> > >
>>> >
>>> >
>>> >The code seem to be.. pretty similar to one changeset ahead .. hmm?-)
>>> >
>>> >   Do you mind to encapsulate his logic to new class in textextensions? Those
>>> >   two testcases can then use this class.
>> sure
>>
>>> >In ideal world of APIs:
>>> >
>>> >public class DeploymentPropetiesModifier {
>>> >
>>> >public DeploymentPropetiesModifier(){
>>> >this (PathsAndFiles.USER_DEPLOYMENT_FILE)
>>> >}
>>> >DeploymentPropetiesModifier(InfrastructureDescriptor src){
>>> >    this.src=src;
>>> >}
>>> >
>>> >public void setProperties(ReasonableInput i){
>>> >    if (setPropertiesCalled) throw new IllegalStateException
>>> >   ReasonableReturn originalState  setPropertiesToDeploymentConfig(i,
>>> >   src.file){
>>> >   saveState(originalState)
>>> >}
>>> >
>>> >
>>> >public void restoreProperties(){
>>> >    if (!setPropertiesCalled) throw new IllegalStateException
>>> >    setPropertiesToDeploymentConfig(savedFile, src.getFile){
>>> >
>>> >}
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >public static ReasonableReturn
>>> >setPropertiesToDeploymentConfig(ReasonableInput i, File config){
>>> >      the actual logic :)
>>> >}
>>> >
>>> >
>>> >
>>> >public static ReasonableReturn
>>> >setPropertiesToDeploymentConfig(ReasonableInput i){
>>> >     return setPropertiesToDeploymentConfig(i,
>>> >     PathsAndFiles.USER_DEPLOYMENT_FILE.getFile());
>>> >}
>>> >}
>>> >
>>> >maybe you will need to have separate methods for set and restore.. but I hope
>>> >not.
>>> >
>>> >
>>> >Also please try to avoid hardcoded string like
>>> >deployment.manifest.attributes.check=CODEBASE. Use the variables rather
>>> >both. Not in impl, but in calls to impl :)
>>> >
>>> >
>>> >
>>> >Then the calls will be
>>> >
>>> >
>>> >in testextensions-tests -
>>> >DeploymentPropetiesModifierTest{
>>> >
>>> >test  ReasonableReturn setPropertiesToDeploymentConfig(ReasonableInput i,
>>> >File config)s
>>> >test  DeploymentPropetiesModifier(tmpSource)s
>>> >
>>> >}
>>> >
>>> >
>>> >in calling class:
>>> >
>>> >   @BeforeClass
>>> >     public static void setupDeploymentProperties() throws IOException {
>>> >deploymentPropetiesModifier = new DeploymentPropetiesModifier();
>>> >deploymentPropetiesModifier.setProperties({KEY_ENABLE_MANIFEST_ATTRIBUTES_CHECK,MANIFEST_ATTRIBUTES_CHECK.whatever})
>>>
>>> >[*]
>>> >}
>>> >
>>> >[*]if that will occure to many times, feel free to intorduce shortcyt method
>>> >in DeploymentPropetiesModifier
>>> >...
>>> >public void setMAnifestAttributesToWhatever(){
>>> >     setProperties({KEY_ENABLE_MANIFEST_ATTRIBUTES_CHECK,MANIFEST_ATTRIBUTES_CHECK.whatever)
>>> >}
>>> >...
>>> >
>>> >     @AfterClass
>>> >      public static void setbackDeploymentProperties() throws IOException {
>>> >deploymentPropetiesModifier.restoreProperties()
>>> >}
>>> >
>>> >
>>> >Whole thos eis maybe overkill, but right thing to do.
>>> >
>>> >
>>> >IF you will follow my ideas,t ry to come with better names and not so complex
>>> >ReasonableInput/Output
>> I have attached a patch with the new class. I have also changed the tests to use this
>> new class in the patch.
>>
>> I was not sure where to put the new class in test-extensions, so I put it in the jnlp.tools package,
>> where would be a good place to put it ?
>>
>> Thank you,
>> Lukasz Dracz
>>
>>
>>
>>
>>
>>
>> deploymentPropertiesModifier-2.patch
>>
>>
>> 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
>>
>> @@ -49,6 +49,7 @@
>>   import net.sourceforge.jnlp.closinglisteners.AutoOkClosingListener;
>>
>>   import net.sourceforge.jnlp.config.PathsAndFiles;
>> +import net.sourceforge.jnlp.tools.DeploymentPropetiesModifier;
>>   import net.sourceforge.jnlp.util.FileUtils;
>>   import org.junit.AfterClass;
>>   import org.junit.BeforeClass;
>> @@ -64,50 +65,22 @@
>>       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 DeploymentPropetiesModifier permissionsModifier;
>> +    private static DeploymentPropetiesModifier securityLevelModifier;
>>
>>       @BeforeClass
>>       public static void setupDeploymentProperties() throws IOException {
>> -        deployFile = PathsAndFiles.USER_DEPLOYMENT_FILE.getFile();
>> -        String properties = FileUtils.loadFileAsString(deployFile);
>> +        permissionsModifier = new DeploymentPropetiesModifier();
>> +        permissionsModifier.setProperties("deployment.manifest.attributes.check", "PERMISSIONS");
>>
>> -        for (String line : properties.split("\n")) {
>> -            if (line.contains("deployment.manifest.attribute.check")) {
>> -                attributesCheck = line;
>> -                properties = properties.replace(line,
>> "deployment.manifest.attributes.check=PERMISSIONS\n");
>> -            }
>> -            if (line.contains("deployment.security.level")) {
>> -                securityLevel = line;
>> -                properties = properties.replace(line, "deployment.security.level=ALLOW_UNSIGNED\n");
>> -            }
>> -        }
>> -        if (attributesCheck == null) {
>> -            properties += "deployment.manifest.attributes.check=PERMISSIONS\n";
>> -        }
>> -        if (securityLevel == null) {
>> -            properties += "deployment.security.level=ALLOW_UNSIGNED\n";
>> -        }
>> -
>> -        FileUtils.saveFile(properties, deployFile);
>> +        securityLevelModifier = new DeploymentPropetiesModifier();
>> +        securityLevelModifier.setProperties("deployment.security.level", "ALLOW_UNSIGNED");
>
> please don't use hardcoded values. Use variabels for both key and value.
>
>>       }
>>
>>       @AfterClass
>>       public static void setbackDeploymentProperties() throws IOException {
>> -        String properties = FileUtils.loadFileAsString(deployFile);
>> -        if (attributesCheck != null) {
>> -            properties = properties.replace("deployment.manifest.attributes.check=PERMISSIONS\n",
>> attributesCheck);
>> -        } else {
>> -            properties = properties.replace("deployment.manifest.attributes.check=PERMISSIONS\n",
>> "");
>> -        }
>> -
>> -        if (securityLevel != null) {
>> -            properties = properties.replace("deployment.security.level=ALLOW_UNSIGNED\n",
>> securityLevel);
>> -        } else {
>> -            properties = properties.replace("deployment.security.level=ALLOW_UNSIGNED\n", "");
>> -        }
>> -        FileUtils.saveFile(properties, deployFile);
>> +        permissionsModifier.restoreProperties();
>> +        securityLevelModifier.restoreProperties();
>>       }
>>
>>       @Test
>> diff --git
>> a/tests/reproducers/signed/CodeBaseManifestEntrySignedMatching/testcases/CodeBaseManifestEntrySignedMatching.java
>> b/tests/reproducers/signed/CodeBaseManifestEntrySignedMatching/testcases/CodeBaseManifestEntrySignedMatching.java
>>
>> ---
>> a/tests/reproducers/signed/CodeBaseManifestEntrySignedMatching/testcases/CodeBaseManifestEntrySignedMatching.java
>>
>> +++
>> b/tests/reproducers/signed/CodeBaseManifestEntrySignedMatching/testcases/CodeBaseManifestEntrySignedMatching.java
>>
>> @@ -49,8 +49,11 @@
>>   import net.sourceforge.jnlp.browsertesting.Browsers;
>>   import net.sourceforge.jnlp.closinglisteners.AutoOkClosingListener;
>>   import net.sourceforge.jnlp.closinglisteners.RulesFolowingClosingListener;
>> +import net.sourceforge.jnlp.tools.DeploymentPropetiesModifier;
>>   import net.sourceforge.jnlp.util.FileUtils;
>> +import org.junit.AfterClass;
>>   import org.junit.Assert;
>> +import org.junit.BeforeClass;
>>   import org.junit.Test;
>>
>>   public class CodeBaseManifestEntrySignedMatching extends BrowserTest {
>> @@ -67,6 +70,19 @@
>>           /*5*/ "CBCheckSignedAppletDontMatchException",
>>           /*6*/ "CBCheckSignedFail"};
>>
>> +    private static DeploymentPropetiesModifier codebaseModifier;
>> +
>> +    @BeforeClass
>> +    public static void setupDeploymentProperties() throws IOException {
>> +        codebaseModifier = new DeploymentPropetiesModifier();
>> +        codebaseModifier.setProperties("deployment.manifest.attributes.check", "CODEBASE");
>
> please don't use hardcoded values. Use variabels for both key and value.
>
>> +    }
>> +
>> +    @AfterClass
>> +    public static void setbackDeploymentProperties() throws IOException {
>> +        codebaseModifier.restoreProperties();
>> +    }
>> +
>>       public static String getMessage(int i) {
>>           try {
>>               String s = "";//_cs, _de, _pl
>> 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
>>
>> @@ -37,9 +37,7 @@
>>
>>   import static org.junit.Assert.assertTrue;
>>
>> -import java.io.File;
>>   import java.io.IOException;
>> -import java.util.List;
>>
>>   import net.sourceforge.jnlp.ProcessResult;
>>   import net.sourceforge.jnlp.annotations.Bug;
>> @@ -48,10 +46,8 @@
>>   import net.sourceforge.jnlp.browsertesting.BrowserTest;
>>   import net.sourceforge.jnlp.browsertesting.Browsers;
>>   import net.sourceforge.jnlp.closinglisteners.AutoOkClosingListener;
>> +import net.sourceforge.jnlp.tools.DeploymentPropetiesModifier;
>>
>> -import net.sourceforge.jnlp.config.PathsAndFiles;
>> -import net.sourceforge.jnlp.runtime.ManifestAttributesChecker;
>> -import net.sourceforge.jnlp.util.FileUtils;
>>   import org.junit.AfterClass;
>>   import org.junit.BeforeClass;
>>   import org.junit.Test;
>> @@ -66,37 +62,17 @@
>>       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 DeploymentPropetiesModifier deploymentPropetiesModifier;
>>
>>       @BeforeClass
>>       public static void setupDeploymentProperties() throws IOException {
>> -        deployFile = PathsAndFiles.USER_DEPLOYMENT_FILE.getFile();
>> -        String properties = FileUtils.loadFileAsString(deployFile);
>> -
>> -        for (String line : properties.split("\n")) {
>> -            if (line.contains("deployment.manifest.attribute.check")) {
>> -                attributesCheck = line;
>> -                properties = properties.replace(line,
>> "deployment.manifest.attributes.check=PERMISSIONS\n");
>> -            }
>> -        }
>> -        if (attributesCheck == null) {
>> -            properties += "deployment.manifest.attributes.check=PERMISSIONS\n";
>> -        }
>> -
>> -        FileUtils.saveFile(properties, deployFile);
>> +        deploymentPropetiesModifier = new DeploymentPropetiesModifier();
>> +        deploymentPropetiesModifier.setProperties("deployment.manifest.attributes.check",
>> "PERMISSIONS");
>>       }
>>
>>       @AfterClass
>>       public static void setbackDeploymentProperties() throws IOException {
>> -        String properties = FileUtils.loadFileAsString(deployFile);
>> -        if (attributesCheck != null) {
>> -            properties = properties.replace("deployment.manifest.attributes.check=PERMISSIONS\n",
>> attributesCheck);
>> -        } else {
>> -            properties = properties.replace("deployment.manifest.attributes.check=PERMISSIONS\n",
>> "");
>> -        }
>> -
>> -        FileUtils.saveFile(properties, deployFile);
>> +        deploymentPropetiesModifier.restoreProperties();
>>       }
>>
>>       @Test
>> diff --git a/tests/test-extensions/net/sourceforge/jnlp/tools/DeploymentPropetiesModifier.java
>> b/tests/test-extensions/net/sourceforge/jnlp/tools/DeploymentPropetiesModifier.java
>> new file mode 100644
>> --- /dev/null
>> +++ b/tests/test-extensions/net/sourceforge/jnlp/tools/DeploymentPropetiesModifier.java
>> @@ -0,0 +1,110 @@
>> +/*
>> +   Copyright (C) 2015 Red Hat, Inc.
>> +
>> +This file is part of IcedTea.
>> +
>> +IcedTea is free software; you can redistribute it and/or
>> +modify it under the terms of the GNU General Public License as published by
>> +the Free Software Foundation, version 2.
>> +
>> +IcedTea is distributed in the hope that it will be useful,
>> +but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +General Public License for more details.
>> +
>> +You should have received a copy of the GNU General Public License
>> +along with IcedTea; see the file COPYING.  If not, write to
>> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>> +02110-1301 USA.
>> +
>> +Linking this library statically or dynamically with other modules is
>> +making a combined work based on this library.  Thus, the terms and
>> +conditions of the GNU General Public License cover the whole
>> +combination.
>> +
>> +As a special exception, the copyright holders of this library give you
>> +permission to link this library with independent modules to produce an
>> +executable, regardless of the license terms of these independent
>> +modules, and to copy and distribute the resulting executable under
>> +terms of your choice, provided that you also meet, for each linked
>> +independent module, the terms and conditions of the license of that
>> +module.  An independent module is a module which is not derived from
>> +or based on this library.  If you modify this library, you may extend
>> +this exception to your version of the library, but you are not
>> +obligated to do so.  If you do not wish to do so, delete this
>> +exception statement from your version.
>> +*/
>> +
>> +package net.sourceforge.jnlp.tools;
>> +
>> +import java.io.IOException;
>> +
>> +import net.sourceforge.jnlp.config.PathsAndFiles;
>> +import net.sourceforge.jnlp.util.FileUtils;
>> +
>> +public class DeploymentPropetiesModifier {
>> +
>> +    private final PathsAndFiles.InfrastructureFileDescriptor src;
>> +    private String savedValue;
>> +    private String requestedProperty;
>> +    private String requestedValue;
>> +    private boolean isPropertiesSet;
>> +
>> +    public DeploymentPropetiesModifier() {
>> +        this(PathsAndFiles.USER_DEPLOYMENT_FILE);
>> +    }
>> +
>> +    public DeploymentPropetiesModifier(PathsAndFiles.InfrastructureFileDescriptor src) {
>> +        this.src = src;
>> +        isPropertiesSet = false;
>> +    }
>> +
>> +    public void setProperties(String property, String value) throws IOException {
>
>
> As I understand it, it can change only one property in time. (and I like it as it is simple enough)
> Please rename it to setProperty
>
>> +        if (isPropertiesSet) {
>> +            throw new IllegalStateException();
>> +        }
>> +        isPropertiesSet = true;
>> +        requestedProperty = property;
>> +        requestedValue = value;
>> +
>> +        setDeploymentProperties(requestedProperty, requestedValue);
>> +    }
>> +
>> +    public void restoreProperties() throws IOException {
>> +        if (!isPropertiesSet) {
>> +            throw new IllegalStateException();
>> +        }
>> +        isPropertiesSet = false;
>> +
>> +        restoreDeploymentProperties();
>> +    }
>> +
>> +    private void setDeploymentProperties(String property, String value) throws IOException {
>> +        String properties = FileUtils.loadFileAsString(src.getFile());
>> +
>> +        for (String line : properties.split("\n")) {
>> +            if (line.contains(property)) {
>> +                savedValue = line;
>> +                properties = properties.replace(line, property + "=" + value + "\n");
>> +            }
>> +        }
>> +
>> +        if (savedValue == null) {
>> +            properties += property + "=" + value + "\n";
>> +        }
>> +
>> +        FileUtils.saveFile(properties, src.getFile());
>> +    }
>> +
>> +    private void restoreDeploymentProperties() throws IOException {
>> +        String properties = FileUtils.loadFileAsString(src.getFile());
>> +        if (savedValue != null) {
>> +            properties = properties.replace(requestedProperty + "=" + requestedValue + "\n",
>> savedValue);
>> +        } else {
>> +            properties = properties.replace(requestedProperty + "=" + requestedValue + "\n", "");
>> +        }
>> +
>> +        FileUtils.saveFile(properties, src.getFile());
>> +    }
>> +
>> +}
>>
>
> Thank you very much. Nice tool.
>
> After those two and half  changes, ok to head. Feel free to send to one more round of review.
>
>
> hmhm.. can be some test added? There are test-extensions-test for such an purpose O:)
>
> J.
>



More information about the distro-pkg-dev mailing list