[rfc][icedtea-web][policyeditor] Parsing enhancement, unit testing, file-changed checking
Andrew Azores
aazores at redhat.com
Tue Feb 25 06:47:01 PST 2014
On 02/24/2014 08:15 AM, Jiri Vanek wrote:
> On 02/21/2014 05:48 PM, Andrew Azores wrote:
>> Hi,
>>
>> One of the attached patches changes PolicyEditor to use regexes when
>> attempting to convert Strings
>> into PolicyEditorPermissions, and adds plenty of unit tests for
>> various things. The other patch adds
>> functionality so PolicyEditor will warn the user if the file on disk
>> is modified externally in
>> between the time PolicyEditor opened the file and when it is to be
>> saved. This is done by checking
>> the file's MD5, which was added to FileUtils so it's reusable in the
>> future. ChangeLogs are included
>> in the patches. They should apply cleanly together.
>>
>> Thanks,
>>
>
> overall ok, few nits inline:
>
>> --
>> Andrew A
>>
>>
>> policy-editor-filechanged-check.patch
>>
>>
>> diff --git a/ChangeLog b/ChangeLog
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,15 @@
>> +2014-02-21 Andrew Azores<aazores at rehdat.com>
>> +
>> + * netx/net/sourceforge/jnlp/resources/Messages.properties:
>> + (RFileModifiedWhileLoaded, RFileModifiedWhileLoadedDetail,
>> PEFileModified)
>> + new messages for when a loaded file changes on disk
>> + *
>> netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java:
>> + (fileMd5, openAndParsePolicyFile, savePolicyFile) keep track of
>> file MD5,
>> + prompt user if MD5 changes between file load and save.
>> (updateCheckboxes)
>> + avoid an NPE
>> + * netx/net/sourceforge/jnlp/util/FileUtils.java:
>> + (showFileChangedWhileLoadedDialog) new functions
>> +
>> 2014-02-20 Andrew Azores<aazores at redhat.com>
>>
>> New simplified PolicyEditor for editing Java policy files,
>> particularly
>> diff --git a/netx/net/sourceforge/jnlp/resources/Messages.properties
>> b/netx/net/sourceforge/jnlp/resources/Messages.properties
>> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties
>> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties
>> @@ -164,6 +164,8 @@
>> RCantWriteFile=Could not write to file {0}
>> RFileReadOnly=Opening file in read-only mode
>> RExpectedFile=Expected {0} to be a file but it was not
>> +RFileModifiedWhileLoaded=File Modification Warning
>> +RFileModifiedWhileLoadedDetail=The file {0} has been modified on
>> disk since it was loaded. Reload?
>> RRemoveRPermFailed=Removing read permission on file {0} failed
>> RRemoveWPermFailed=Removing write permissions on file {0} failed
>> RRemoveXPermFailed=Removing execute permissions on file {0} failed
>> @@ -481,6 +483,7 @@
>> PEExitMenuItem=Exit
>> PEViewMenu=View
>> PECustomPermissionsItem=Custom Permissions...
>> +PEFileModified=The policy file has been modified since it was
>> opened. Reload and re-edit before saving?
>>
>> # Policy Editor CustomPolicyViewer
>> PECPTitle=Custom Policy Viewer
>> diff --git
>> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>> --- a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>> @@ -161,6 +161,7 @@
>> private final JFileChooser fileChooser;
>> private CustomPolicyViewer cpViewer = null;
>> private final WeakReference<PolicyEditor> weakThis = new
>> WeakReference<PolicyEditor>(this);
>> + private byte[] fileMd5;
>>
>> private final ActionListener okButtonAction, closeButtonAction,
>> addCodebaseButtonAction,
>> removeCodebaseButtonAction, openButtonAction,
>> saveAsButtonAction, viewCustomButtonAction;
>> @@ -546,12 +547,24 @@
>> for (final ActionListener l : box.getActionListeners()) {
>> box.removeActionListener(l);
>> }
>> - box.setSelected(codebasePermissionsMap.get(codebase).get(perm));
>> + final Map<PolicyEditorPermissions, Boolean> map =
>> codebasePermissionsMap.get(codebase);
>> + final boolean state;
>> + if (map != null) {
>> + final Boolean s = map.get(perm);
>> + if (s != null) {
>> + state = s;
>> + } else {
>> + state = false;
>> + }
>> + } else {
>> + state = false;
>> + }
>> + box.setSelected(state);
>> box.addActionListener(new ActionListener() {
>> @Override
>> public void actionPerformed(final ActionEvent e) {
>> changesMade = true;
>> - codebasePermissionsMap.get(codebase).put(perm, box.isSelected());
>> + map.put(perm, box.isSelected());
>> }
>> });
>> }
>> @@ -720,17 +733,21 @@
>> }
>> final String contents;
>> try {
>> - contents = FileUtils.loadFileAsString(new
>> File(filePath));
>> + final File f = new File(filePath);
>> + fileMd5 = FileUtils.getFileMD5Sum(f);
>> + contents = FileUtils.loadFileAsString(f);
>> } catch (final IOException e) {
>> OutputController.getLogger().log(e);
>> OutputController.getLogger().log(OutputController.Level.ERROR_ALL,
>> R("RCantOpenFile", filePath));
>> FileUtils.showCouldNotOpenDialog(weakThis.get(), R("PECouldNotOpen"));
>> return;
>> }
>> + codebasePermissionsMap.clear();
>> + customPermissionsMap.clear();
>> // Split on newlines, both \r\n and \n style, for
>> platform-independence
>> final String[] lines = contents.split("[\\r\\n]+");
>> String codebase = "";
>> - FileLock fileLock = null;
>> + final FileLock fileLock;
>> try {
>> fileLock = FileUtils.getFileLock(filePath,
>> false, true);
>> } catch (final FileNotFoundException e) {
>> @@ -809,6 +826,34 @@
>> new Thread() {
>> @Override
>> public void run() {
>> + if (fileMd5 != null) {
>> + try {
>> + final byte[] newChecksum =
>> FileUtils.getFileMD5Sum(new File(filePath));
>> + if (!Arrays.equals(fileMd5, newChecksum)) {
>> + final int response =
>> FileUtils.showFileChangedWhileLoadedDialog(weakThis.get(),
>> + R("PEFileModified"),
>> R("RFileModifiedWhileLoaded"));
>> + switch (response) {
>> + case JOptionPane.YES_OPTION:
>> + openAndParsePolicyFile();
>> + return;
>> + case JOptionPane.NO_OPTION:
>> + break;
>> + case JOptionPane.CANCEL_OPTION:
>> + return;
>> + default:
>> + break;
>> + }
>> + }
>> + } catch (final FileNotFoundException e) {
>> + OutputController.getLogger().log(e);
>> + showCouldNotSaveDialog();
>
> Why can not we save if file is not found?
>
> - we can not load
> - or we can not save because of not existing path to file
> -?
Yea, I probably should have given a more informative message here. This
case is happening within the case that we have a previous MD5 hash
saved, and are not able to compute a current one because of the
FileNotFound, so the file has been deleted or made otherwise
inaccessible while the PolicyEditor was running. I guess some better
checks can be done here, or maybe we should just continue trying to save
regardless... ?
>
>> + return;
>> + } catch (final IOException e) {
>> + OutputController.getLogger().log(e);
>> + showCouldNotSaveDialog();
>> + return;
>> + }
>> + }
>> final StringBuilder sb = new StringBuilder();
>> sb.append(AUTOGENERATED_NOTICE);
>> sb.append("\n/* Generated by PolicyEditor at " +
>> new SimpleDateFormat("yyyy-MM-dd HH:mm:ss")
>> @@ -839,6 +884,7 @@
>>
>> try {
>> FileUtils.saveFile(sb.toString(), new
>> File(filePath));
>> + fileMd5 = FileUtils.getFileMD5Sum(new
>> File(filePath));
>> changesMade = false;
>> showChangesSavedDialog();
>> } catch (final IOException e) {
>> diff --git a/netx/net/sourceforge/jnlp/util/FileUtils.java
>> b/netx/net/sourceforge/jnlp/util/FileUtils.java
>> --- a/netx/net/sourceforge/jnlp/util/FileUtils.java
>> +++ b/netx/net/sourceforge/jnlp/util/FileUtils.java
>> @@ -30,8 +30,12 @@
>> import java.io.OutputStreamWriter;
>> import java.io.RandomAccessFile;
>> import java.io.Writer;
>> +import java.nio.ByteBuffer;
>> import java.nio.channels.FileChannel;
>> import java.nio.channels.FileLock;
>> +import java.security.DigestInputStream;
>> +import java.security.MessageDigest;
>> +import java.security.NoSuchAlgorithmException;
>> import java.util.ArrayList;
>> import java.util.List;
>>
>> @@ -394,6 +398,27 @@
>> }
>>
>> /**
>> + * Show a message warning the user that a file has been modified
>> on disk since it was loaded and asking to reload
>> + * @param frame a {@link JFrame} to act as parent to this dialog
>> + * @param message the dialog message to display
>> + * @param title the dialog title
>> + * @return the user's choice
>> + */
>> + public static int showFileChangedWhileLoadedDialog(final JFrame
>> frame, final String message, final String title) {
>> + return JOptionPane.showConfirmDialog(frame, message, title,
>> JOptionPane.YES_NO_CANCEL_OPTION, JOptionPane.WARNING_MESSAGE);
>
> Not sure if this generalization belongs here.
>
> If yes, then whole md5sum compression belongs here.
>
> I'm imagining new class around
> netx/net/sourceforge/jnlp/util/FileUtils.java , eg md5sumWatcher. Or
> UpdateWatchingFile
>
> And it will encapsulate all md5sum checks you do PolicyEditor. and
> of course handle the dialogue.
Hmm... this does seem like it could be a useful tool to have. I'll look
into that for the next round of review.
>> + }
>> +
>> + /**
>> + * Show a message warning the user that a file has been modified
>> on disk since it was loaded and asking to reload
>> + * @param frame a {@link JFrame} to act as parent to this dialog
>> + * @param filePath the path to the file
>> + * @return the user's choice
>> + */
>> + public static int showFileChangeWhileLoadedDialog(final JFrame
>> frame, final String filePath) {
>> + return showFileChangedWhileLoadedDialog(frame,
>> R("RFileModifiedWhileLoadedDetail", filePath),
>> R("RFileModifiedWhileLoaded"));
>> + }
>> +
>> + /**
>> * Returns a String that is suitable for using in GUI elements for
>> * displaying (long) paths to users.
>> *
>> @@ -579,4 +604,39 @@
>> public static String loadFileAsString(File f, String encoding)
>> throws IOException {
>> return getContentOfStream(new FileInputStream(f), encoding);
>> }
>> +
>> + public static byte[] getFileMD5Sum(final File f) throws
>> FileNotFoundException, IOException {
>> + final MessageDigest md5;
>> + final InputStream is;
>> + final DigestInputStream dis;
>> + try {
>> + md5 = MessageDigest.getInstance("MD5");
>> + is = new FileInputStream(f);
>> + dis = new DigestInputStream(is, md5);
>> +
>> + md5.update(getContentOfStream(dis).getBytes());
>> + } catch (final NoSuchAlgorithmException e) {
>> + // There definitely should be an MD5 algorithm... if
>> not, well, nothing to do but fail.
>
> Then maybe rather bubble the exception up? Yes log, but rethrow. Or at
> least let user know. Or use timestamp as fall back.
>
> This return null is suspicious and smell by NPE later. ( I mean also
> in changesets time horizon later)
Rethrow as a RuntimeException perhaps? It doesn't seem right to make
NoSuchAlgorithm a checked exception in this case.
>
>> + OutputController.getLogger().log(e);
>> + return null;
>> + }
>> +
>> + try {
>> + if (is != null) {
>> + is.close();
>> + }
>> + } catch (final IOException e) {
>> + OutputController.getLogger().log(e);
> blah :)) To match try/catch -> log
>
> Something better here?
>
> Eg utility method which throw sensible information, Which is catched
> in proper place with proper reaction?
Just trying to close some Streams here... what is there really to do if
closing the steam fails?
>
>> + }
>> +
>> + try {
>> + if (dis != null) {
>> + dis.close();
>> + }
>> + } catch (final IOException e) {
>> + OutputController.getLogger().log(e);
>> + }
>> +
>> + return md5.digest();
>> + }
>> }
>>
>>
>> policy-editor-parsing-tests.patch
>>
>>
>> diff --git a/ChangeLog b/ChangeLog
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,24 @@
>> +2014-02-21 Andrew Azores<aazores at redhat.com>
>> +
>> + * NEWS: added entry for PolicyEditor
>> + *
>> netx/net/sourceforge/jnlp/security/policyeditor/CustomPermission.java:
>> + (fromString) use regexes to parse
>> + *
>> netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java:
>> + (getPermissions) returns empty map rather than null.
>> + (getCustomPermissions) new function. (openAndParsePolicyFile)
>> check for
>> + OpenFileResult FAILURE and NOT_FILE rather than null
>> + * netx/net/sourceforge/jnlp/util/FileUtils.java:
>> + (testDirectoryPermissions) add check for getCanonicalFile and null
>> + safeguarding. (testFilePermissions) add check for
>> getCanonicalFile and
>> + return FAILURE rather than null
>> + *
>> tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/CustomPermissionTest.java:
>> + (testMissingQuotationMarks) new test
>> + *
>> tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/PolicyEditorTest.java:
>> + (testReturnedCustomPermissionsSetIsCopy,
>> + testCodebaseTrailingSlashesDoNotMatch) new tests
>> + *
>> tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/PolicyEditorParsingTest.java:
>> + new tests
>> +
>> 2014-02-20 Andrew Azores<aazores at redhat.com>
>>
>> New simplified PolicyEditor for editing Java policy files,
>> particularly
>> diff --git a/NEWS b/NEWS
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -16,6 +16,7 @@
>> * Dialogs center on screen before becoming visible
>> * Support for u45 and u51 new manifest attributes
>> (Application-Name, Codebase)
>> * Custom applet permission policies panel in itweb-settings control
>> panel
>> +* New PolicyEditor for easily adding/removing permissions to
>> individual applets
>> * Cache Viewer
>> - Can be closed by ESC key
>> - Enabling and disabling of operational buttons is handled properly
>> diff --git
>> a/netx/net/sourceforge/jnlp/security/policyeditor/CustomPermission.java
>> b/netx/net/sourceforge/jnlp/security/policyeditor/CustomPermission.java
>> ---
>> a/netx/net/sourceforge/jnlp/security/policyeditor/CustomPermission.java
>> +++
>> b/netx/net/sourceforge/jnlp/security/policyeditor/CustomPermission.java
>> @@ -36,6 +36,9 @@
>>
>> package net.sourceforge.jnlp.security.policyeditor;
>>
>> +import java.util.regex.Matcher;
>> +import java.util.regex.Pattern;
>> +
>> /**
>> * This class models a permission entry in a policy file which is
>> not included
>> * in the default set of permissions used by the PolicyEditor, ie,
>> permissions
>> @@ -63,16 +66,15 @@
>> * @return a CustomPermission representing this string
>> */
>> public static CustomPermission fromString(final String string) {
>> - final String[] parts = string.trim().split(" ");
>> - if (!parts[0].equals("permission") || parts.length < 3 ||
>> !string.trim().endsWith(";")) {
>
> The pattenr should be precompiled, and then reused. Pelase extract the
> variable.
> Also some comments would be nice :))
> What is expected to be parsed and what not.
Yea, I realized this and started working on it right after I sent this
for review :/
>
>> + final Pattern pattern =
>> +
>> Pattern.compile("permission\\s+([\\w.]+)\\s+\"([^\"]+)\",?\\s*\"?([^\"]*)\"?;");
> Isn't the regex to benevolent?
Yea, probably.
>
>> + final Matcher matcher = pattern.matcher(string);
>> + if (!matcher.matches()) {
>> return null;
>> }
>> - final String typeStr = removeQuotes(parts[1]);
>> - final String targetStr =
>> removeQuotes(removeSemicolon(removeComma(parts[2])));
>> - String actionsStr = "";
>> - if (parts.length > 3) {
>> - actionsStr = removeQuotes(removeSemicolon(parts[3]));
>> - }
>> + final String typeStr = matcher.group(1);
>> + final String targetStr = matcher.group(2);
>> + final String actionsStr = matcher.group(3);
>> return new CustomPermission(typeStr, targetStr, actionsStr);
>> }
>>
>> @@ -96,18 +98,6 @@
>> return sb.toString();
>> }
>>
>> - private static String removeQuotes(final String string) {
>> - return string.replaceAll("\"", "");
>> - }
>> -
>> - private static String removeSemicolon(final String string) {
>> - return string.replaceAll(";", "");
>> - }
>> -
>> - private static String removeComma(final String string) {
>> - return string.replaceAll(",", "");
>> - }
>> -
>> @Override
>> public int compareTo(final CustomPermission o) {
>> if (this == o) {
>> diff --git
>> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>> --- a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>> @@ -56,6 +56,7 @@
>> import java.util.Arrays;
>> import java.util.Calendar;
>> import java.util.Collection;
>> +import java.util.Collections;
>> import java.util.HashMap;
>> import java.util.HashSet;
>> import java.util.List;
>> @@ -533,7 +534,29 @@
>> * @return a map of permissions to whether these permissions
>> are set for the given codebase
>> */
>> public Map<PolicyEditorPermissions, Boolean>
>> getPermissions(final String codebase) {
>> - return new HashMap<PolicyEditorPermissions,
>> Boolean>(codebasePermissionsMap.get(codebase));
>> + final Map<PolicyEditorPermissions, Boolean> permissions =
>> codebasePermissionsMap.get(codebase);
>> + if (permissions != null) {
>> + return new HashMap<PolicyEditorPermissions,
>> Boolean>(permissions);
>> + } else {
>> + final Map<PolicyEditorPermissions, Boolean> blank = new
>> HashMap<PolicyEditorPermissions, Boolean>();
>> + for (final PolicyEditorPermissions perm :
>> PolicyEditorPermissions.values()) {
>> + blank.put(perm, false);
>> + }
>> + return blank;
>> + }
>> + }
>> +
>> + /**
>> + * @param codebase the codebase to query
>> + * @return a collection of CustomPermissions granted to the
>> given codebase
>> + */
>> + public Collection<CustomPermission> getCustomPermissions(final
>> String codebase) {
>> + final Collection<CustomPermission> permissions =
>> customPermissionsMap.get(codebase);
>> + if (permissions != null) {
>> + return new HashSet<CustomPermission>(permissions);
>> + } else {
>> + return Collections.emptySet();
>> + }
>> }
>>
>> /**
>> @@ -711,7 +734,7 @@
>> @Override
>> public void run() {
>> OpenFileResult ofr =
>> FileUtils.testFilePermissions(new File(filePath));
>> - if (ofr == null) {
>> + if (ofr == OpenFileResult.FAILURE || ofr ==
>> OpenFileResult.NOT_FILE) {
>> FileUtils.showCouldNotOpenFilepathDialog(weakThis.get(), filePath);
>> return;
>> }
>> diff --git a/netx/net/sourceforge/jnlp/util/FileUtils.java
>> b/netx/net/sourceforge/jnlp/util/FileUtils.java
>> --- a/netx/net/sourceforge/jnlp/util/FileUtils.java
>> +++ b/netx/net/sourceforge/jnlp/util/FileUtils.java
>> @@ -285,8 +285,13 @@
>> * @param file the {@link File} representing a Java Policy file
>> to test
>> * @return a {@link DirectoryCheckResults} object representing
>> the results of the test
>> */
>> - public static DirectoryCheckResults
>> testDirectoryPermissions(final File file) {
>> - if (file == null || !file.getParentFile().exists()) {
>> + public static DirectoryCheckResults
>> testDirectoryPermissions(File file) {
>> + try {
>> + file = file.getCanonicalFile();
>> + } catch (final IOException e) {
>
> Never consume exceptions. never.
Oops :( my mistake, didn't mean to completely disregard the exception.
>> + return null;
>> + }
>> + if (file == null || file.getParentFile() == null ||
>> !file.getParentFile().exists()) {
>> return null;
>> }
>> final List<File> policyDirectory = new ArrayList<File>();
>> @@ -303,11 +308,15 @@
>> * as an empty plain file.
>> * @param file the {@link File} to verify
>> * @return an {@link OpenFileResult} representing the
>> accessibility level of the file
>> - * @throws IOException if the file is not accessible
>> */
>> - public static OpenFileResult testFilePermissions(final File file) {
>> + public static OpenFileResult testFilePermissions(File file) {
>> if (file == null || !file.exists()) {
>> - return null;
>> + return OpenFileResult.FAILURE;
>> + }
>> + try {
>> + file = file.getCanonicalFile();
>> + } catch (final IOException e) {
>> + return OpenFileResult.FAILURE;
>> }
>> final DirectoryCheckResults dcr =
>> FileUtils.testDirectoryPermissions(file);
>> if (dcr != null && dcr.getFailures() == 0) {
>> diff --git
>> a/tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/CustomPermissionTest.java
>> b/tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/CustomPermissionTest.java
>>
>> ---
>> a/tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/CustomPermissionTest.java
>> +++
>> b/tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/CustomPermissionTest.java
>> @@ -37,6 +37,7 @@
>> package net.sourceforge.jnlp.security.policyeditor;
>>
>> import static org.junit.Assert.assertTrue;
>> +import net.sourceforge.jnlp.annotations.KnownToFail;
>>
>> import org.junit.Test;
>>
>> @@ -67,6 +68,33 @@
>> }
>>
>> @Test
>> + public void testMissingQuotationMarks() throws Exception {
>> + final CustomPermission cp =
>> CustomPermission.fromString("permission java.io.FilePermission *,
>> read,write;");
>> + assertTrue("Custom permission should be null", cp == null);
>> + }
>> +
>> + @KnownToFail
>> + @Test
>> + public void testMissingPunctuationForActions() throws Exception {
>> + final String missingComma = "permission
>> java.io.FilePermission \"*\" \"read,write\";";
>> + final String missingFirstQuote = "permission
>> java.io.FilePermission \"*\", read,write\";";
>> + final String missingSecondQuote = "permission
>> java.io.FilePermission \"*\", \"read,write;";
>> + final String missingBothQuotes = "permission
>> java.io.FilePermission \"*\", read,write;";
>> + final String missingAll = "permission java.io.FilePermission
>> \"*\" read,write;";
>> +
>> + final CustomPermission cp1 =
>> CustomPermission.fromString(missingComma);
>> + assertTrue("Custom permission for " + missingComma + "
>> should be null", cp1 == null);
>> + final CustomPermission cp2 =
>> CustomPermission.fromString(missingFirstQuote);
>> + assertTrue("Custom permission for " + missingFirstQuote + "
>> should be null", cp2 == null);
>> + final CustomPermission cp3 =
>> CustomPermission.fromString(missingSecondQuote);
>> + assertTrue("Custom permission for " + missingSecondQuote + "
>> should be null", cp3 == null);
>> + final CustomPermission cp4 =
>> CustomPermission.fromString(missingBothQuotes);
>> + assertTrue("Custom permission for " + missingBothQuotes + "
>> should be null", cp4 == null);
>> + final CustomPermission cp5 =
>> CustomPermission.fromString(missingAll);
>> + assertTrue("Custom permission for " + missingAll + " should
>> be null", cp5 == null);
>> + }
>> +
>
> This test needs to be splitted - probably. Rigth nopw I believe ALL
> the asserts fails. However, only first of them is executed.
> On the other hand, I donot wont to have 6 new KnownToFail unittests :-/
I agree.
>
>> + @Test
>> public void testToString() throws Exception {
>> final CustomPermission cp = new
>> CustomPermission("java.io.FilePermission", "*", "read");
>> final String expected = "permission java.io.FilePermission
>> \"*\", \"read\";";
>> diff --git
>> a/tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/PolicyEditorParsingTest.java
>> b/tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/PolicyEditorParsingTest.java
>>
>> new file mode 100644
>> --- /dev/null
>> +++
>> b/tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/PolicyEditorParsingTest.java
>> @@ -0,0 +1,231 @@
>> +/*Copyright (C) 2014 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.
>> + */
>> +
>> +import static org.junit.Assert.assertFalse;
>> +import static org.junit.Assert.assertTrue;
>> +
>> +import java.io.File;
>> +import java.io.FileInputStream;
>> +import java.util.Map;
>> +import net.sourceforge.jnlp.annotations.KnownToFail;
>> +import net.sourceforge.jnlp.security.policyeditor.PolicyEditor;
>> +import
>> net.sourceforge.jnlp.security.policyeditor.PolicyEditorPermissions;
>> +import net.sourceforge.jnlp.util.FileUtils;
>> +import org.junit.After;
>> +import org.junit.Before;
>> +import org.junit.Test;
>> +
>> +public class PolicyEditorParsingTest {
>> +
>> + private File file;
>> + private PolicyEditor editor;
>> + private Map<PolicyEditorPermissions, Boolean> permissions;
>> +
>> + private static final String LINEBREAK =
>> System.getProperty("line.separator");
>> +
>> + private static final String READ_PERMISSION = "permission
>> java.io.FilePermission \"${user.home}${/}*\", \"read\";";
>> + private static final String WRITE_PERMISSION = "permission
>> java.io.FilePermission \"${user.home}${/}*\", \"write\";";
>> +
>> + private static final String NORMAL_POLICY = "grant {" + LINEBREAK
>> + + "\t" + READ_PERMISSION + LINEBREAK
>> + + "};" + LINEBREAK;
>> +
>> + private static final String CODEBASE_POLICY = "grant codeBase
>> \"http://redhat.com\" {" + LINEBREAK
>> + + "\t" + READ_PERMISSION + LINEBREAK
>> + + "};" + LINEBREAK;
>> +
>> + private static final String MULTIPLE_PERMISSION_POLICY = "grant
>> {" + LINEBREAK
>> + + "\t" + READ_PERMISSION + LINEBREAK
>> + + "\t" + WRITE_PERMISSION + LINEBREAK
>> + + "};" + LINEBREAK;
>> +
>> + private static final String COMMENT_HEADER = "/* TEST COMMENT
>> */" + LINEBREAK;
>> +
>> + private static final String COMMENT_BLOCKED_PERMISSION = "grant
>> {" + LINEBREAK
>> + + "\t/*" + READ_PERMISSION + "*/" + LINEBREAK
>> + + "};" + LINEBREAK;
>> +
>> + private static final String COMMENT_BLOCKED_POLICY = "/*" +
>> NORMAL_POLICY + "*/" + LINEBREAK;
>> +
>> + private static final String COMMENTED_PERMISSION = "grant {" +
>> LINEBREAK
>> + + "\t//" + READ_PERMISSION + LINEBREAK
>> + + "};" + LINEBREAK;
>> +
>> + private static final String COMMENT_AFTER_PERMISSION = "grant {"
>> + LINEBREAK
>> + + "\t" + READ_PERMISSION + " // comment" + LINEBREAK
>> + + "};" + LINEBREAK;
>> +
>> + private static final String MISSING_WHITESPACE_POLICY = "grant {
>> " + READ_PERMISSION + " };";
>> +
>> + private static final String MULTIPLE_PERMISSIONS_PER_LINE =
>> "grant {" + LINEBREAK
>> + + "\t" + READ_PERMISSION + " " + WRITE_PERMISSION +
>> LINEBREAK
>> + + "};" + LINEBREAK;
>> +
>> + @Before
>> + public void createTempFile() throws Exception {
>> + file = File.createTempFile("PolicyEditor", ".policy");
>> + file.deleteOnExit();
>> + }
>
> The tests for no file, jsut string parsding exists, correct?
Hmm? Each test in this reproducer writes a mock policy to a temp file,
has the editor load and parse this file, and checks on the results.
>
>> +
>> + private void setupTest(final String policyContents, final String
>> codebase) throws Exception {
>> + FileUtils.saveFile(policyContents, file);
>> + editor = PolicyEditor.createInstance(file.getCanonicalPath());
>> + Thread.sleep(100); // policy editor loads asynch, give it
>> some time to populate
>> + permissions = editor.getPermissions(codebase);
>> + }
>> +
>> + @Test
>> + public void testNormalPolicy() throws Exception {
>> + setupTest(NORMAL_POLICY, "");
>> + assertTrue("Permissions should include READ_LOCAL_FILES",
>> permissions.get(PolicyEditorPermissions.READ_LOCAL_FILES));
>> + for (final PolicyEditorPermissions perm :
>> permissions.keySet()) {
>> + if
>> (!perm.equals(PolicyEditorPermissions.READ_LOCAL_FILES)) {
>> + assertFalse("Permission " + perm + " should not be
>> granted", permissions.get(perm));
>> + }
>> + }
>> + }
>> +
>> + @Test
>> + public void testCommentHeaders() throws Exception {
>> + setupTest(COMMENT_HEADER, "");
>> + for (final PolicyEditorPermissions perm :
>> permissions.keySet()) {
>> + assertFalse("Permission " + perm + " should not be
>> granted", permissions.get(perm));
>> + }
>> + }
>> +
>> + @Test
>> + public void testCommentBlockedPermission() throws Exception {
>> + setupTest(COMMENT_BLOCKED_PERMISSION, "");
>> + for (final PolicyEditorPermissions perm :
>> permissions.keySet()) {
>> + assertFalse("Permission " + perm + " should not be
>> granted", permissions.get(perm));
>> + }
>> + }
>> +
>> + @KnownToFail
>> + @Test
>> + public void testCommentBlockedPolicy() throws Exception {
>> + setupTest(COMMENT_BLOCKED_POLICY, "");
>> + for (final PolicyEditorPermissions perm :
>> permissions.keySet()) {
>> + assertFalse("Permission " + perm + " should not be
>> granted", permissions.get(perm));
>> + }
>> + }
>> +
>> + @Test
>> + public void testCommentedLine() throws Exception {
>> + setupTest(COMMENTED_PERMISSION, "");
>> + for (final PolicyEditorPermissions perm :
>> permissions.keySet()) {
>> + assertFalse("Permission " + perm + " should not be
>> granted", permissions.get(perm));
>> + }
>> + }
>> +
>> + @Test
>> + public void testMultiplePermissions() throws Exception {
>> + setupTest(MULTIPLE_PERMISSION_POLICY, "");
>> +
>> + assertTrue("Permissions should include READ_LOCAL_FILES",
>> permissions.get(PolicyEditorPermissions.READ_LOCAL_FILES));
>> + assertTrue("Permissions should include WRITE_LOCAL_FILES",
>> permissions.get(PolicyEditorPermissions.WRITE_LOCAL_FILES));
>> + for (final PolicyEditorPermissions perm :
>> permissions.keySet()) {
>> + if
>> (!perm.equals(PolicyEditorPermissions.READ_LOCAL_FILES) &&
>> !perm.equals(PolicyEditorPermissions.WRITE_LOCAL_FILES)) {
>> + assertFalse("Permission " + perm + " should not be
>> granted", permissions.get(perm));
>> + }
>> + }
>> + }
>> +
>> + @KnownToFail
>> + @Test
>> + public void testMultiplePermissionsPerLine() throws Exception {
>> + setupTest(MULTIPLE_PERMISSIONS_PER_LINE, "");
>> +
>> + assertTrue("Permissions should include READ_LOCAL_FILES",
>> permissions.get(PolicyEditorPermissions.READ_LOCAL_FILES));
>> + assertTrue("Permissions should include WRITE_LOCAL_FILES",
>> permissions.get(PolicyEditorPermissions.WRITE_LOCAL_FILES));
>> + for (final PolicyEditorPermissions perm :
>> permissions.keySet()) {
>> + if
>> (!perm.equals(PolicyEditorPermissions.READ_LOCAL_FILES) &&
>> !perm.equals(PolicyEditorPermissions.WRITE_LOCAL_FILES)) {
>> + assertFalse("Permission " + perm + " should not be
>> granted", permissions.get(perm));
>> + }
>> + }
>> + }
>> +
>> + @KnownToFail
>> + @Test
>> + public void testMissingWhitespace() throws Exception {
>> + setupTest(MISSING_WHITESPACE_POLICY, "");
>> +
>> + assertTrue("Permissions should include READ_LOCAL_FILES",
>> permissions.get(PolicyEditorPermissions.READ_LOCAL_FILES));
>> + for (final PolicyEditorPermissions perm :
>> permissions.keySet()) {
>> + if
>> (!perm.equals(PolicyEditorPermissions.READ_LOCAL_FILES)) {
>> + assertFalse("Permission " + perm + " should not be
>> granted", permissions.get(perm));
>> + }
>> + }
>> + }
>> +
>> + @Test
>> + public void testPolicyWithCodebase() throws Exception {
>> + setupTest(CODEBASE_POLICY,"http://redhat.com");
>> +
>> + assertTrue("Permissions should include READ_LOCAL_FILES",
>> permissions.get(PolicyEditorPermissions.READ_LOCAL_FILES));
>> + for (final PolicyEditorPermissions perm :
>> permissions.keySet()) {
>> + if
>> (!perm.equals(PolicyEditorPermissions.READ_LOCAL_FILES)) {
>> + assertFalse("Permission " + perm + " should not be
>> granted", permissions.get(perm));
>> + }
>> + }
>> + }
>> +
>> + @Test
>> + public void testCodebaseTrailingSlashesDoNotMatch() throws
>> Exception {
>> + // note the trailing '/' - looks like the same URL but is
>> not. JDK PolicyTool considers these as
>> + // different codeBases, so so does PolicyEditor
>> + setupTest(CODEBASE_POLICY,"http://redhat.com/");
>> +
>> + for (final PolicyEditorPermissions perm :
>> permissions.keySet()) {
>> + if
>> (!perm.equals(PolicyEditorPermissions.READ_LOCAL_FILES)) {
>> + assertFalse("Permission " + perm + " should not be
>> granted", permissions.get(perm));
>> + }
>> + }
>> + }
>> +
>> + @Test
>> + public void testCommentAfterPermission() throws Exception {
>> + setupTest(COMMENT_AFTER_PERMISSION, "");
>> +
>> + assertTrue("Permissions should include READ_LOCAL_FILES",
>> permissions.get(PolicyEditorPermissions.READ_LOCAL_FILES));
>> + for (final PolicyEditorPermissions perm :
>> permissions.keySet()) {
>> + if
>> (!perm.equals(PolicyEditorPermissions.READ_LOCAL_FILES)) {
>> + assertFalse("Permission " + perm + " should not be
>> granted", permissions.get(perm));
>> + }
>> + }
>> + }
>> +
>> +}
>> diff --git
>> a/tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/PolicyEditorTest.java
>> b/tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/PolicyEditorTest.java
>>
>> ---
>> a/tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/PolicyEditorTest.java
>> +++
>> b/tests/netx/unit/net/sourceforge/jnlp/security/policyeditor/PolicyEditorTest.java
>> @@ -122,6 +122,14 @@
>> }
>>
>> @Test
>> + public void testReturnedCustomPermissionsSetIsCopy() throws
>> Exception {
>> + final Collection<CustomPermission> original =
>> editor.getCustomPermissions("");
>> + original.add(new CustomPermission("java.io.FilePermission",
>> "*", "write"));
>> + final Collection<CustomPermission> second =
>> editor.getCustomPermissions("");
>> + assertTrue("There should not be any custom permissions",
>> second.isEmpty());
>> + }
>> +
>> + @Test
>> public void testDefaultPermissionsAllFalse() throws Exception {
>> final Map<PolicyEditorPermissions, Boolean> defaultMap =
>> editor.getPermissions("");
>> editor.addNewCodebase("http://redhat.com");
>> @@ -152,6 +160,19 @@
>> }
>>
>> @Test
>> + public void testCodebaseTrailingSlashesDoNotMatch() throws
>> Exception {
>> + final Set<String> toAdd = new HashSet<String>();
>> + toAdd.add("http://redhat.com");
>> + toAdd.add("http://redhat.com/");
>
> I wouold recomand to replace redaht url by some unreal url like
> testing.url or whatever.
Alright.
>
>> + editor.addNewCodebases(toAdd);
>> + final Collection<String> codebases = editor.getCodebases();
>> + assertTrue("Editor should have default codebase",
>> codebases.contains(""));
>> + for (final String codebase : toAdd) {
>> + assertTrue("Editor should have " + codebase,
>> codebases.contains(codebase));
>> + }
>> + }
>> +
>> + @Test
>> public void testArgsToMap() throws Exception {
>> final String[] args = new String[] {
>> "-codebase","http://redhat.com
>> http://icedtea.classpath.org",
>>
>
> Thanx!
>
> I must ask - how run in sandbox is going?
>
> It is main blocker for 1.5. (+my new attributes :( but those are not
> so much monolitic)
>
> J.
>
http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2014-February/026218.html
Thanks,
--
Andrew A
More information about the distro-pkg-dev
mailing list