From 2e985cf1fe96295540f779d2ea8b1548cc49acaa Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Fri, 1 Feb 2019 11:50:06 +1000 Subject: [PATCH 1/2] throw FileNotFoundException if permissions config file does not exist --- .../PermissioningConfigurationBuilder.java | 13 ++++--- .../pegasys/pantheon/cli/PantheonCommand.java | 37 +++++++++++-------- ...PermissioningConfigurationBuilderTest.java | 12 ++++++ .../pantheon/cli/PantheonCommandTest.java | 11 ++++++ 4 files changed, 52 insertions(+), 21 deletions(-) diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/PermissioningConfigurationBuilder.java b/pantheon/src/main/java/tech/pegasys/pantheon/PermissioningConfigurationBuilder.java index 67593fb7d1..c1f0601a8c 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/PermissioningConfigurationBuilder.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/PermissioningConfigurationBuilder.java @@ -18,6 +18,7 @@ import tech.pegasys.pantheon.ethereum.permissioning.PermissioningConfiguration; import java.io.File; +import java.io.FileNotFoundException; import java.net.URI; import java.util.ArrayList; import java.util.List; @@ -37,7 +38,8 @@ public class PermissioningConfigurationBuilder { public static PermissioningConfiguration permissioningConfigurationFromToml( final String configFilePath, final boolean permissionedNodeEnabled, - final boolean permissionedAccountEnabled) { + final boolean permissionedAccountEnabled) + throws FileNotFoundException { return permissioningConfiguration( configFilePath, permissionedNodeEnabled, permissionedAccountEnabled); @@ -46,7 +48,8 @@ public static PermissioningConfiguration permissioningConfigurationFromToml( public static PermissioningConfiguration permissioningConfiguration( final String configFilePath, final boolean permissionedNodeEnabled, - final boolean permissionedAccountEnabled) { + final boolean permissionedAccountEnabled) + throws FileNotFoundException { TomlParseResult permToml = TomlConfigFileParser.loadConfiguration( @@ -97,14 +100,14 @@ private static String permissioningConfigTomlAsString(final File file) { } } - private static File permissioningConfigFile(final String filename) { + private static File permissioningConfigFile(final String filename) throws FileNotFoundException { final File permissioningConfigFile = new File(filename); if (permissioningConfigFile.exists()) { return permissioningConfigFile; } else { - LOG.error("File does not exist: permissioning config path: {}.", filename); - return null; + throw new FileNotFoundException( + "File does not exist: permissioning config path: " + filename); } } } diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java index 8863676ecf..5c6fa38bf1 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java @@ -56,6 +56,7 @@ import tech.pegasys.pantheon.util.bytes.BytesValue; import java.io.File; +import java.io.FileNotFoundException; import java.io.IOException; import java.net.InetAddress; import java.net.URI; @@ -583,21 +584,25 @@ public void run() { } final EthNetworkConfig ethNetworkConfig = updateNetworkConfig(getNetwork()); - final PermissioningConfiguration permissioningConfiguration = permissioningConfiguration(); - ensureAllBootnodesAreInWhitelist(ethNetworkConfig, permissioningConfiguration); - - synchronize( - buildController(), - p2pEnabled, - peerDiscoveryEnabled, - ethNetworkConfig.getBootNodes(), - maxPeers, - p2pHost, - p2pPort, - jsonRpcConfiguration(), - webSocketConfiguration(), - metricsConfiguration(), - permissioningConfiguration); + try { + final PermissioningConfiguration permissioningConfiguration = permissioningConfiguration(); + ensureAllBootnodesAreInWhitelist(ethNetworkConfig, permissioningConfiguration); + + synchronize( + buildController(), + p2pEnabled, + peerDiscoveryEnabled, + ethNetworkConfig.getBootNodes(), + maxPeers, + p2pHost, + p2pPort, + jsonRpcConfiguration(), + webSocketConfiguration(), + metricsConfiguration(), + permissioningConfiguration); + } catch (FileNotFoundException e) { + throw new ParameterException(new CommandLine(this), e.getMessage()); + } } private NetworkName getNetwork() { @@ -732,7 +737,7 @@ MetricsConfiguration metricsConfiguration() { return metricsConfiguration; } - private PermissioningConfiguration permissioningConfiguration() { + private PermissioningConfiguration permissioningConfiguration() throws FileNotFoundException { if (!permissionsAccountsEnabled && !permissionsNodesEnabled) { return PermissioningConfiguration.createDefault(); diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/PermissioningConfigurationBuilderTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/PermissioningConfigurationBuilderTest.java index 71c7cdc673..ec7b67ccac 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/PermissioningConfigurationBuilderTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/PermissioningConfigurationBuilderTest.java @@ -168,4 +168,16 @@ public void permissioningConfigFromFileMustSetFilePath() throws IOException { assertThat(permissioningConfiguration.isAccountWhitelistSet()).isTrue(); assertThat(permissioningConfiguration.getAccountWhitelist()).isEmpty(); } + + @Test + public void permissioningConfigFromNonexistentFileMustThrowException() throws IOException { + + try { + PermissioningConfigurationBuilder.permissioningConfigurationFromToml( + "file-does-not-exist", true, true); + fail("expected exception because file does not exist"); + } catch (Exception e) { + assertThat(e.getMessage().contains("File does not exist")).isTrue(); + } + } } diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java index fa556d39c5..204edb143b 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java @@ -319,6 +319,17 @@ public void permissionsTomlPathWithoutOptionMustDisplayUsage() { assertThat(commandOutput.toString()).isEmpty(); } + @Test + public void permissionsEnabledWithNonexistentConfigFileMustError() { + parseCommand( + "--permissions-accounts-enabled", "--permissions-config-path", "file-does-not-exist"); + + verifyZeroInteractions(mockRunnerBuilder); + + assertThat(commandErrorOutput.toString()).contains("File does not exist"); + assertThat(commandOutput.toString()).isEmpty(); + } + @Test public void permissionsTomlPathMustUseOption() throws IOException { From 7f0a0fd79024ce15d986874fe2136b07edd9daf9 Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Fri, 1 Feb 2019 13:49:05 +1000 Subject: [PATCH 2/2] exception handling, error messages, edge cases, tests --- .../PermissioningConfigurationBuilder.java | 43 ++++++----- .../pantheon/TomlConfigFileParser.java | 12 +-- .../pegasys/pantheon/cli/PantheonCommand.java | 5 +- ...PermissioningConfigurationBuilderTest.java | 77 ++++++++++++++----- ...permissioning_config_unrecognized_key.toml | 3 + 5 files changed, 88 insertions(+), 52 deletions(-) create mode 100644 pantheon/src/test/resources/permissioning_config_unrecognized_key.toml diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/PermissioningConfigurationBuilder.java b/pantheon/src/main/java/tech/pegasys/pantheon/PermissioningConfigurationBuilder.java index c1f0601a8c..2f0ffc4fee 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/PermissioningConfigurationBuilder.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/PermissioningConfigurationBuilder.java @@ -20,26 +20,24 @@ import java.io.File; import java.io.FileNotFoundException; import java.net.URI; -import java.util.ArrayList; import java.util.List; import java.util.stream.Collectors; import com.google.common.io.Resources; import net.consensys.cava.toml.TomlArray; import net.consensys.cava.toml.TomlParseResult; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; public class PermissioningConfigurationBuilder { - private static final Logger LOG = LogManager.getLogger(); + public static final String ACCOUNTS_WHITELIST = "accounts-whitelist"; + public static final String NODES_WHITELIST = "nodes-whitelist"; // will be used to reload the config from a file while node is running public static PermissioningConfiguration permissioningConfigurationFromToml( final String configFilePath, final boolean permissionedNodeEnabled, final boolean permissionedAccountEnabled) - throws FileNotFoundException { + throws Exception { return permissioningConfiguration( configFilePath, permissionedNodeEnabled, permissionedAccountEnabled); @@ -49,14 +47,22 @@ public static PermissioningConfiguration permissioningConfiguration( final String configFilePath, final boolean permissionedNodeEnabled, final boolean permissionedAccountEnabled) - throws FileNotFoundException { + throws Exception { - TomlParseResult permToml = - TomlConfigFileParser.loadConfiguration( - permissioningConfigTomlAsString(permissioningConfigFile(configFilePath))); + TomlParseResult permToml; + boolean foundValidOptions = false; - TomlArray accountWhitelistTomlArray = permToml.getArray("accounts-whitelist"); - TomlArray nodeWhitelistTomlArray = permToml.getArray("nodes-whitelist"); + try { + permToml = + TomlConfigFileParser.loadConfiguration( + permissioningConfigTomlAsString(permissioningConfigFile(configFilePath))); + } catch (Exception e) { + throw new Exception( + "Unable to read permissions TOML config file : " + configFilePath + " " + e.getMessage()); + } + + TomlArray accountWhitelistTomlArray = permToml.getArray(ACCOUNTS_WHITELIST); + TomlArray nodeWhitelistTomlArray = permToml.getArray(NODES_WHITELIST); final PermissioningConfiguration permissioningConfiguration = PermissioningConfiguration.createDefault(); @@ -71,7 +77,8 @@ public static PermissioningConfiguration permissioningConfiguration( .collect(Collectors.toList()); permissioningConfiguration.setAccountWhitelist(accountsWhitelistToml); } else { - permissioningConfiguration.setAccountWhitelist(new ArrayList<>()); + throw new Exception( + ACCOUNTS_WHITELIST + " config option missing in TOML config file " + configFilePath); } } if (permissionedNodeEnabled) { @@ -85,19 +92,15 @@ public static PermissioningConfiguration permissioningConfiguration( .collect(Collectors.toList()); permissioningConfiguration.setNodeWhitelist(nodesWhitelistToml); } else { - permissioningConfiguration.setNodeWhitelist(new ArrayList<>()); + throw new Exception( + NODES_WHITELIST + " config option missing in TOML config file " + configFilePath); } } return permissioningConfiguration; } - private static String permissioningConfigTomlAsString(final File file) { - try { - return Resources.toString(file.toURI().toURL(), UTF_8); - } catch (final Exception e) { - LOG.error("Unable to load permissioning config {}.", file, e); - return null; - } + private static String permissioningConfigTomlAsString(final File file) throws Exception { + return Resources.toString(file.toURI().toURL(), UTF_8); } private static File permissioningConfigFile(final String filename) throws FileNotFoundException { diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/TomlConfigFileParser.java b/pantheon/src/main/java/tech/pegasys/pantheon/TomlConfigFileParser.java index 65e3dd8f83..b24d84f288 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/TomlConfigFileParser.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/TomlConfigFileParser.java @@ -17,29 +17,25 @@ import net.consensys.cava.toml.Toml; import net.consensys.cava.toml.TomlParseError; import net.consensys.cava.toml.TomlParseResult; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; public class TomlConfigFileParser { - private static final Logger LOG = LogManager.getLogger(); - private static TomlParseResult checkConfigurationValidity( - final TomlParseResult result, final String toml) { + final TomlParseResult result, final String toml) throws Exception { if (result == null || result.isEmpty()) { - LOG.error("Unable to read TOML configuration file %s", toml); + throw new Exception("Empty TOML result: " + toml); } return result; } - public static TomlParseResult loadConfiguration(final String toml) { + public static TomlParseResult loadConfiguration(final String toml) throws Exception { TomlParseResult result = Toml.parse(toml); if (result.hasErrors()) { final String errors = result.errors().stream().map(TomlParseError::toString).collect(Collectors.joining("%n")); ; - LOG.error("Invalid TOML configuration : %s", errors); + throw new Exception("Invalid TOML configuration : " + errors); } return checkConfigurationValidity(result, toml); diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java index 5c6fa38bf1..5bf792c5de 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java @@ -56,7 +56,6 @@ import tech.pegasys.pantheon.util.bytes.BytesValue; import java.io.File; -import java.io.FileNotFoundException; import java.io.IOException; import java.net.InetAddress; import java.net.URI; @@ -600,7 +599,7 @@ public void run() { webSocketConfiguration(), metricsConfiguration(), permissioningConfiguration); - } catch (FileNotFoundException e) { + } catch (Exception e) { throw new ParameterException(new CommandLine(this), e.getMessage()); } } @@ -737,7 +736,7 @@ MetricsConfiguration metricsConfiguration() { return metricsConfiguration; } - private PermissioningConfiguration permissioningConfiguration() throws FileNotFoundException { + private PermissioningConfiguration permissioningConfiguration() throws Exception { if (!permissionsAccountsEnabled && !permissionsNodesEnabled) { return PermissioningConfiguration.createDefault(); diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/PermissioningConfigurationBuilderTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/PermissioningConfigurationBuilderTest.java index ec7b67ccac..9cd49b78c0 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/PermissioningConfigurationBuilderTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/PermissioningConfigurationBuilderTest.java @@ -28,7 +28,7 @@ public class PermissioningConfigurationBuilderTest { - static final String PERMISSIONING_CONFIG_TOML = "permissioning_config.toml"; + static final String PERMISSIONING_CONFIG_VALID = "permissioning_config.toml"; static final String PERMISSIONING_CONFIG_ACCOUNT_WHITELIST_ONLY = "permissioning_config_account_whitelist_only.toml"; static final String PERMISSIONING_CONFIG_NODE_WHITELIST_ONLY = @@ -39,17 +39,19 @@ public class PermissioningConfigurationBuilderTest { "permissioning_config_empty_whitelists.toml"; static final String PERMISSIONING_CONFIG_ABSENT_WHITELISTS = "permissioning_config_absent_whitelists.toml"; + static final String PERMISSIONING_CONFIG_UNRECOGNIZED_KEY = + "permissioning_config_unrecognized_key.toml"; private final String VALID_NODE_ID = "6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"; @Test - public void permissioningConfig() throws IOException { + public void permissioningConfig() throws Exception { final String uri = "enode://" + VALID_NODE_ID + "@192.168.0.9:4567"; final String uri2 = "enode://" + VALID_NODE_ID + "@192.169.0.9:4568"; - final URL configFile = Resources.getResource(PERMISSIONING_CONFIG_TOML); + final URL configFile = Resources.getResource(PERMISSIONING_CONFIG_VALID); final Path toml = Files.createTempFile("toml", ""); Files.write(toml, Resources.toByteArray(configFile)); @@ -66,7 +68,7 @@ public void permissioningConfig() throws IOException { } @Test - public void permissioningConfigWithOnlyNodeWhitelistSet() throws IOException { + public void permissioningConfigWithOnlyNodeWhitelistSet() throws Exception { final String uri = "enode://" + VALID_NODE_ID + "@192.168.0.9:4567"; @@ -84,7 +86,7 @@ public void permissioningConfigWithOnlyNodeWhitelistSet() throws IOException { } @Test - public void permissioningConfigWithOnlyAccountWhitelistSet() throws IOException { + public void permissioningConfigWithOnlyAccountWhitelistSet() throws Exception { final URL configFile = Resources.getResource(PERMISSIONING_CONFIG_ACCOUNT_WHITELIST_ONLY); final Path toml = Files.createTempFile("toml", ""); @@ -101,7 +103,7 @@ public void permissioningConfigWithOnlyAccountWhitelistSet() throws IOException } @Test - public void permissioningConfigWithInvalidEnode() throws IOException { + public void permissioningConfigWithInvalidEnode() throws Exception { final URL configFile = Resources.getResource(PERMISSIONING_CONFIG_INVALID_ENODE); final Path toml = Files.createTempFile("toml", ""); @@ -117,7 +119,7 @@ public void permissioningConfigWithInvalidEnode() throws IOException { } @Test - public void permissioningConfigWithEmptyWhitelistMustNotError() throws IOException { + public void permissioningConfigWithEmptyWhitelistMustNotError() throws Exception { final URL configFile = Resources.getResource(PERMISSIONING_CONFIG_EMPTY_WHITELISTS); final Path toml = Files.createTempFile("toml", ""); @@ -134,26 +136,59 @@ public void permissioningConfigWithEmptyWhitelistMustNotError() throws IOExcepti } @Test - public void permissioningConfigWithAbsentWhitelistMustSetValues() throws IOException { + public void permissioningConfigWithAbsentWhitelistMustThrowException() throws Exception { final URL configFile = Resources.getResource(PERMISSIONING_CONFIG_ABSENT_WHITELISTS); final Path toml = Files.createTempFile("toml", ""); Files.write(toml, Resources.toByteArray(configFile)); - PermissioningConfiguration permissioningConfiguration = - PermissioningConfigurationBuilder.permissioningConfiguration( - toml.toAbsolutePath().toString(), true, true); + try { + PermissioningConfigurationBuilder.permissioningConfiguration( + toml.toAbsolutePath().toString(), true, true); + fail("expected exception: no valid whitelists in the TOML file"); + } catch (Exception e) { + assertThat(e.getMessage().contains("Unexpected end of line")).isTrue(); + } + } - assertThat(permissioningConfiguration.isNodeWhitelistSet()).isTrue(); - assertThat(permissioningConfiguration.getNodeWhitelist()).isEmpty(); - assertThat(permissioningConfiguration.isAccountWhitelistSet()).isTrue(); - assertThat(permissioningConfiguration.getAccountWhitelist()).isEmpty(); + @Test + public void permissioningConfigWithUnrecognizedKeyMustThrowException() throws Exception { + + final URL configFile = Resources.getResource(PERMISSIONING_CONFIG_UNRECOGNIZED_KEY); + final Path toml = Files.createTempFile("toml", ""); + Files.write(toml, Resources.toByteArray(configFile)); + + try { + PermissioningConfigurationBuilder.permissioningConfiguration( + toml.toAbsolutePath().toString(), true, true); + fail("expected exception: didn't find a recognized key in the TOML file"); + } catch (Exception e) { + assertThat(e.getMessage().contains("config option missing")).isTrue(); + assertThat(e.getMessage().contains(PermissioningConfigurationBuilder.ACCOUNTS_WHITELIST)) + .isTrue(); + } } @Test - public void permissioningConfigFromFileMustSetFilePath() throws IOException { + public void permissioningConfigWithEmptyFileMustThrowException() throws Exception { - final URL configFile = Resources.getResource(PERMISSIONING_CONFIG_ABSENT_WHITELISTS); + // write an empty file + final Path toml = Files.createTempFile("toml", ""); + + try { + PermissioningConfigurationBuilder.permissioningConfiguration( + toml.toAbsolutePath().toString(), true, true); + fail("expected exception: empty TOML file"); + + } catch (Exception e) { + assertThat(e.getMessage().contains("Empty TOML result")).isTrue(); + } + } + + @Test + public void permissioningConfigFromFileMustSetFilePath() throws Exception { + + final URL configFile = Resources.getResource(PERMISSIONING_CONFIG_VALID); final Path toml = Files.createTempFile("toml", ""); Files.write(toml, Resources.toByteArray(configFile)); @@ -163,10 +198,10 @@ public void permissioningConfigFromFileMustSetFilePath() throws IOException { assertThat(permissioningConfiguration.getConfigurationFilePath()).isEqualTo(toml.toString()); - assertThat(permissioningConfiguration.isNodeWhitelistSet()).isTrue(); - assertThat(permissioningConfiguration.getNodeWhitelist()).isEmpty(); assertThat(permissioningConfiguration.isAccountWhitelistSet()).isTrue(); - assertThat(permissioningConfiguration.getAccountWhitelist()).isEmpty(); + assertThat(permissioningConfiguration.getAccountWhitelist()) + .containsExactly("0x0000000000000000000000000000000000000009"); + assertThat(permissioningConfiguration.isNodeWhitelistSet()).isTrue(); } @Test @@ -175,7 +210,7 @@ public void permissioningConfigFromNonexistentFileMustThrowException() throws IO try { PermissioningConfigurationBuilder.permissioningConfigurationFromToml( "file-does-not-exist", true, true); - fail("expected exception because file does not exist"); + fail("expected exception: file does not exist"); } catch (Exception e) { assertThat(e.getMessage().contains("File does not exist")).isTrue(); } diff --git a/pantheon/src/test/resources/permissioning_config_unrecognized_key.toml b/pantheon/src/test/resources/permissioning_config_unrecognized_key.toml new file mode 100644 index 0000000000..aa54b4ab70 --- /dev/null +++ b/pantheon/src/test/resources/permissioning_config_unrecognized_key.toml @@ -0,0 +1,3 @@ +# Permissioning TOML file with typo in key + +perm-node-whitelist=[] \ No newline at end of file