[rfc][icedtea-web][policyeditor] Parsing enhancement, unit testing, file-changed checking
Jiri Vanek
jvanek at redhat.com
Mon Feb 24 05:15:29 PST 2014
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
-?
> + 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.
> + }
> +
> + /**
> + * 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)
> + 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?
> + }
> +
> + 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.
> + final Pattern pattern =
> + Pattern.compile("permission\\s+([\\w.]+)\\s+\"([^\"]+)\",?\\s*\"?([^\"]*)\"?;");
Isn't the regex to benevolent?
> + 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.
> + 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 :-/
> + @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?
> +
> + 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.
> + 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.
More information about the distro-pkg-dev
mailing list