From 655c8663d7ccc1f6f3a1b7e1bfb0e969ae2c4542 Mon Sep 17 00:00:00 2001 From: norrislee Date: Mon, 22 Apr 2024 17:10:10 -0700 Subject: [PATCH 01/19] Easy logging improvements --- .../client/config/SFClientConfig.java | 9 ++ .../client/config/SFClientConfigParser.java | 59 +++++++++- .../jdbc/DefaultSFConnectionHandler.java | 104 +++++++++++++++--- 3 files changed, 151 insertions(+), 21 deletions(-) diff --git a/src/main/java/net/snowflake/client/config/SFClientConfig.java b/src/main/java/net/snowflake/client/config/SFClientConfig.java index 1029b1167..bc631339e 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfig.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfig.java @@ -1,7 +1,10 @@ package net.snowflake.client.config; +import com.fasterxml.jackson.annotation.JsonAnySetter; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; +import java.util.LinkedHashMap; +import java.util.Map; import java.util.Objects; /** POJO class for Snowflake's client config. */ @@ -57,6 +60,8 @@ public static class CommonProps { @JsonProperty("log_path") private String logPath; + @JsonAnySetter private Map unknownKeys = new LinkedHashMap<>(); + public CommonProps() {} public void CommonProps(String logLevel, String logPath) { @@ -80,6 +85,10 @@ public void setLogPath(String logPath) { this.logPath = logPath; } + public Map getUnknownKeys() { + return unknownKeys; + } + @Override public boolean equals(Object o) { if (this == o) { diff --git a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java index 2f3ee3b91..31cff8e14 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java @@ -8,8 +8,12 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Paths; +import java.nio.file.attribute.PosixFilePermission; +import java.util.Map; +import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; +import net.snowflake.client.core.Constants; import net.snowflake.client.jdbc.SnowflakeDriver; import net.snowflake.client.log.SFLogger; import net.snowflake.client.log.SFLoggerFactory; @@ -33,33 +37,80 @@ public class SFClientConfigParser { * @return SFClientConfig */ public static SFClientConfig loadSFClientConfig(String configFilePath) throws IOException { + if (configFilePath != null) { + logger.info( + String.format("Attempting to enable easy logging with file path %s", configFilePath)); + } String derivedConfigFilePath = null; if (configFilePath != null && !configFilePath.isEmpty()) { // 1. Try to read the file at configFilePath. derivedConfigFilePath = configFilePath; + logger.info( + String.format( + "Using client configuration path from a connection string: %s", + derivedConfigFilePath)); } else if (System.getenv().containsKey(SF_CLIENT_CONFIG_ENV_NAME)) { // 2. If SF_CLIENT_CONFIG_ENV_NAME is set, read from env. derivedConfigFilePath = systemGetEnv(SF_CLIENT_CONFIG_ENV_NAME); + logger.info( + String.format( + "Using client configuration path from an environment variable: %s", + derivedConfigFilePath)); } else { // 3. Read SF_CLIENT_CONFIG_FILE_NAME from where jdbc jar is loaded. String driverLocation = Paths.get(getConfigFilePathFromJDBCJarLocation(), SF_CLIENT_CONFIG_FILE_NAME).toString(); if (Files.exists(Paths.get(driverLocation))) { derivedConfigFilePath = driverLocation; + logger.info( + String.format( + "Using client configuration path from jar directory: %s", derivedConfigFilePath)); } else { // 4. Read SF_CLIENT_CONFIG_FILE_NAME if it is present in user home directory. - String userHomeFilePath = - Paths.get(systemGetProperty("user.home"), SF_CLIENT_CONFIG_FILE_NAME).toString(); - if (Files.exists(Paths.get(userHomeFilePath))) { - derivedConfigFilePath = userHomeFilePath; + String homePath = systemGetProperty("user.home"); + if (homePath != null) { + String userHomeFilePath = Paths.get(homePath, SF_CLIENT_CONFIG_FILE_NAME).toString(); + if (Files.exists(Paths.get(userHomeFilePath))) { + derivedConfigFilePath = userHomeFilePath; + logger.info( + String.format( + "Using client configuration path from home directory: %s", + derivedConfigFilePath)); + } } } } if (derivedConfigFilePath != null) { try { + if (Constants.getOS() != Constants.OS.WINDOWS) { + // Check permissions of config file + Set folderPermissions = + Files.getPosixFilePermissions(Paths.get(derivedConfigFilePath)); + if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE) + || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE)) { + String error = + String.format( + "Error due to other users having permission to modify the config file: %s", + derivedConfigFilePath); + throw new IOException(error); + } + } File configFile = new File(derivedConfigFilePath); ObjectMapper objectMapper = new ObjectMapper(); SFClientConfig clientConfig = objectMapper.readValue(configFile, SFClientConfig.class); + logger.info( + String.format( + "Reading values logLevel %s and logPath %s from client configuration", + clientConfig.getCommonProps().getLogLevel(), + clientConfig.getCommonProps().getLogPath())); + + for (Map.Entry prop : + clientConfig.getCommonProps().getUnknownKeys().entrySet()) { + logger.info( + String.format( + "Unknown configuration entry: %s with value: %s", + prop.getKey(), prop.getValue())); + } clientConfig.setConfigFilePath(derivedConfigFilePath); return clientConfig; diff --git a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java index 7ada3a803..d6a80b14d 100644 --- a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java +++ b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java @@ -8,22 +8,19 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; import java.sql.ResultSet; import java.sql.SQLException; import java.sql.SQLNonTransientConnectionException; import java.sql.Statement; import java.util.Map; import java.util.Properties; +import java.util.Set; import java.util.logging.Level; import net.snowflake.client.config.SFClientConfig; import net.snowflake.client.config.SFClientConfigParser; -import net.snowflake.client.core.SFBaseResultSet; -import net.snowflake.client.core.SFBaseSession; -import net.snowflake.client.core.SFBaseStatement; -import net.snowflake.client.core.SFException; -import net.snowflake.client.core.SFSession; -import net.snowflake.client.core.SFSessionProperty; -import net.snowflake.client.core.SFStatement; +import net.snowflake.client.core.*; import net.snowflake.client.jdbc.telemetryOOB.TelemetryService; import net.snowflake.client.log.JDK14Logger; import net.snowflake.client.log.SFLogLevel; @@ -136,13 +133,16 @@ private void setClientConfig() throws SnowflakeSQLLoggedException { String clientConfigFilePath = (String) connectionPropertiesMap.getOrDefault(SFSessionProperty.CLIENT_CONFIG_FILE, null); - SFClientConfig sfClientConfig; - try { - sfClientConfig = SFClientConfigParser.loadSFClientConfig(clientConfigFilePath); - } catch (IOException e) { - throw new SnowflakeSQLLoggedException(sfSession, ErrorCode.INTERNAL_ERROR, e.getMessage()); + SFClientConfig sfClientConfig = sfSession.getSfClientConfig(); + if (sfClientConfig == null) { + try { + sfClientConfig = SFClientConfigParser.loadSFClientConfig(clientConfigFilePath); + } catch (IOException e) { + throw new SnowflakeSQLLoggedException( + sfSession, ErrorCode.INTERNAL_ERROR, e.getMessage(), e.getCause()); + } + sfSession.setSfClientConfig(sfClientConfig); } - sfSession.setSfClientConfig(sfClientConfig); } /** @@ -181,6 +181,9 @@ && systemGetProperty("java.util.logging.config.file") == null) { if (logLevel != null && logPattern != null) { try { + logger.info( + String.format( + "Setting logger with log level %s and log pattern %s", logLevel, logPattern)); JDK14Logger.instantiateLogger(logLevel, logPattern); } catch (IOException ex) { throw new SnowflakeSQLLoggedException( @@ -206,11 +209,13 @@ private String constructLogPattern(String logPathFromConfig) throws SnowflakeSQL String logPattern = "%t/snowflake_jdbc%u.log"; // java.tmpdir + Path logPath; if (logPathFromConfig != null && !logPathFromConfig.isEmpty()) { - Path path = Paths.get(logPathFromConfig, "jdbc"); - if (!Files.exists(path)) { + // Get log path from configuration + logPath = Paths.get(logPathFromConfig); + if (!Files.exists(logPath)) { try { - Files.createDirectories(path); + Files.createDirectories(logPath); } catch (IOException ex) { throw new SnowflakeSQLLoggedException( sfSession, @@ -220,11 +225,76 @@ private String constructLogPattern(String logPathFromConfig) throws SnowflakeSQL logPathFromConfig, ex.getMessage())); } } - logPattern = Paths.get(path.toString(), "snowflake_jdbc%u.log").toString(); + } else { + // Get log path from home directory + String homePath = systemGetProperty("user.home"); + if (homePath == null || homePath.isEmpty()) { + throw new SnowflakeSQLLoggedException( + sfSession, + ErrorCode.INTERNAL_ERROR, + String.format( + "Log path not set in configfile %s and home directory not set.", + logPathFromConfig)); + } + logPath = Paths.get(homePath); } + + Path path = CreateLogPathSubDirectory(logPath); + + logPattern = Paths.get(path.toString(), "snowflake_jdbc%u.log").toString(); return logPattern; } + private Path CreateLogPathSubDirectory(Path logPath) throws SnowflakeSQLLoggedException { + Path path = Paths.get(logPath.toString(), "jdbc"); + if (!Files.exists(path)) { + // Create jdbc subfolder + try { + if (Constants.getOS() == Constants.OS.WINDOWS) { + Files.createDirectories(path); + } else { + Files.createDirectories( + path, + PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"))); + } + } catch (IOException ex) { + throw new SnowflakeSQLLoggedException( + sfSession, + ErrorCode.INTERNAL_ERROR, + String.format( + "Un-able to create jdbc subfolder in configfile %s ,%s", + logPath.toString(), ex.getMessage())); + } + } else { + // Check if jdbc subfolder has correct permissions + if (Constants.getOS() != Constants.OS.WINDOWS) { + try { + Set folderPermissions = Files.getPosixFilePermissions(path); + if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE) + || folderPermissions.contains(PosixFilePermission.GROUP_READ) + || folderPermissions.contains(PosixFilePermission.GROUP_EXECUTE) + || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE) + || folderPermissions.contains(PosixFilePermission.OTHERS_READ) + || folderPermissions.contains(PosixFilePermission.OTHERS_EXECUTE)) { + logger.info( + String.format( + "Access permission for the logs directory '%s' is currently %s and is potentially " + + "accessible to users other than the owner of the logs directory.", + path.toString(), folderPermissions.toString())); + } + } catch (IOException ex) { + throw new SnowflakeSQLLoggedException( + sfSession, + ErrorCode.INTERNAL_ERROR, + String.format( + "Un-able to get permissions of log directory %s ,%s", + path.toString(), ex.getMessage())); + } + } + } + return path; + } + private void initSessionProperties(SnowflakeConnectString conStr, String appID, String appVersion) throws SFException { Map properties = mergeProperties(conStr); From 0d96d6b58e4938e4ed10b4d0b4e1e3e507e80c3a Mon Sep 17 00:00:00 2001 From: norrislee Date: Mon, 22 Apr 2024 17:17:22 -0700 Subject: [PATCH 02/19] Fix import to remove * --- .../snowflake/client/jdbc/DefaultSFConnectionHandler.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java index d6a80b14d..be7074b04 100644 --- a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java +++ b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java @@ -20,7 +20,13 @@ import java.util.logging.Level; import net.snowflake.client.config.SFClientConfig; import net.snowflake.client.config.SFClientConfigParser; -import net.snowflake.client.core.*; +import net.snowflake.client.core.SFBaseResultSet; +import net.snowflake.client.core.SFBaseSession; +import net.snowflake.client.core.SFBaseStatement; +import net.snowflake.client.core.SFException; +import net.snowflake.client.core.SFSession; +import net.snowflake.client.core.SFSessionProperty; +import net.snowflake.client.core.SFStatement; import net.snowflake.client.jdbc.telemetryOOB.TelemetryService; import net.snowflake.client.log.JDK14Logger; import net.snowflake.client.log.SFLogLevel; From a662adc84c2b7c281ab0738751be482b65c257c4 Mon Sep 17 00:00:00 2001 From: norrislee Date: Mon, 22 Apr 2024 17:24:05 -0700 Subject: [PATCH 03/19] Fix imports --- .../net/snowflake/client/jdbc/DefaultSFConnectionHandler.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java index be7074b04..b7486f112 100644 --- a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java +++ b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java @@ -20,6 +20,7 @@ import java.util.logging.Level; import net.snowflake.client.config.SFClientConfig; import net.snowflake.client.config.SFClientConfigParser; +import net.snowflake.client.core.Constants; import net.snowflake.client.core.SFBaseResultSet; import net.snowflake.client.core.SFBaseSession; import net.snowflake.client.core.SFBaseStatement; From 61191f47932a8e8fdd19e09b35098c25c7397571 Mon Sep 17 00:00:00 2001 From: norrislee Date: Fri, 26 Apr 2024 17:59:26 -0700 Subject: [PATCH 04/19] log format changes, add tests --- .../client/config/SFClientConfig.java | 2 +- .../client/config/SFClientConfigParser.java | 30 +++----- .../jdbc/DefaultSFConnectionHandler.java | 27 +++----- .../client/config/SFPermissionsTest.java | 68 +++++++++++++++++++ .../log/JDK14LoggerWithClientLatestIT.java | 57 ++++++++++++++++ 5 files changed, 147 insertions(+), 37 deletions(-) create mode 100644 src/test/java/net/snowflake/client/config/SFPermissionsTest.java diff --git a/src/main/java/net/snowflake/client/config/SFClientConfig.java b/src/main/java/net/snowflake/client/config/SFClientConfig.java index bc631339e..0ed7c81ae 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfig.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfig.java @@ -85,7 +85,7 @@ public void setLogPath(String logPath) { this.logPath = logPath; } - public Map getUnknownKeys() { + protected Map getUnknownKeys() { return unknownKeys; } diff --git a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java index 31cff8e14..4a3614bc1 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java @@ -38,24 +38,20 @@ public class SFClientConfigParser { */ public static SFClientConfig loadSFClientConfig(String configFilePath) throws IOException { if (configFilePath != null) { - logger.info( - String.format("Attempting to enable easy logging with file path %s", configFilePath)); + logger.info("Attempting to enable easy logging with file path {}", configFilePath); } String derivedConfigFilePath = null; if (configFilePath != null && !configFilePath.isEmpty()) { // 1. Try to read the file at configFilePath. derivedConfigFilePath = configFilePath; logger.info( - String.format( - "Using client configuration path from a connection string: %s", - derivedConfigFilePath)); + "Using client configuration path from a connection string: {}", derivedConfigFilePath); } else if (System.getenv().containsKey(SF_CLIENT_CONFIG_ENV_NAME)) { // 2. If SF_CLIENT_CONFIG_ENV_NAME is set, read from env. derivedConfigFilePath = systemGetEnv(SF_CLIENT_CONFIG_ENV_NAME); logger.info( - String.format( - "Using client configuration path from an environment variable: %s", - derivedConfigFilePath)); + "Using client configuration path from an environment variable: {}", + derivedConfigFilePath); } else { // 3. Read SF_CLIENT_CONFIG_FILE_NAME from where jdbc jar is loaded. String driverLocation = @@ -63,8 +59,7 @@ public static SFClientConfig loadSFClientConfig(String configFilePath) throws IO if (Files.exists(Paths.get(driverLocation))) { derivedConfigFilePath = driverLocation; logger.info( - String.format( - "Using client configuration path from jar directory: %s", derivedConfigFilePath)); + "Using client configuration path from jar directory: {}", derivedConfigFilePath); } else { // 4. Read SF_CLIENT_CONFIG_FILE_NAME if it is present in user home directory. String homePath = systemGetProperty("user.home"); @@ -73,9 +68,7 @@ public static SFClientConfig loadSFClientConfig(String configFilePath) throws IO if (Files.exists(Paths.get(userHomeFilePath))) { derivedConfigFilePath = userHomeFilePath; logger.info( - String.format( - "Using client configuration path from home directory: %s", - derivedConfigFilePath)); + "Using client configuration path from home directory: {}", derivedConfigFilePath); } } } @@ -99,17 +92,14 @@ public static SFClientConfig loadSFClientConfig(String configFilePath) throws IO ObjectMapper objectMapper = new ObjectMapper(); SFClientConfig clientConfig = objectMapper.readValue(configFile, SFClientConfig.class); logger.info( - String.format( - "Reading values logLevel %s and logPath %s from client configuration", - clientConfig.getCommonProps().getLogLevel(), - clientConfig.getCommonProps().getLogPath())); + "Reading values logLevel {} and logPath {} from client configuration", + clientConfig.getCommonProps().getLogLevel(), + clientConfig.getCommonProps().getLogPath()); for (Map.Entry prop : clientConfig.getCommonProps().getUnknownKeys().entrySet()) { logger.info( - String.format( - "Unknown configuration entry: %s with value: %s", - prop.getKey(), prop.getValue())); + "Unknown configuration entry: {} with value: {}", prop.getKey(), prop.getValue()); } clientConfig.setConfigFilePath(derivedConfigFilePath); diff --git a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java index b7486f112..32a02f4bd 100644 --- a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java +++ b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java @@ -188,9 +188,7 @@ && systemGetProperty("java.util.logging.config.file") == null) { if (logLevel != null && logPattern != null) { try { - logger.info( - String.format( - "Setting logger with log level %s and log pattern %s", logLevel, logPattern)); + logger.info("Setting logger with log level {} and log pattern {}", logLevel, logPattern); JDK14Logger.instantiateLogger(logLevel, logPattern); } catch (IOException ex) { throw new SnowflakeSQLLoggedException( @@ -198,13 +196,10 @@ && systemGetProperty("java.util.logging.config.file") == null) { } if (sfClientConfig != null) { logger.debug( - String.format( - "SF Client config found at location: %s.", sfClientConfig.getConfigFilePath())); + "SF Client config found at location: {}.", sfClientConfig.getConfigFilePath()); } logger.debug( - String.format( - "Instantiating JDK14Logger with level: %s , output path: %s", - logLevel, logPattern)); + "Instantiating JDK14Logger with level: {}, output path: {}", logLevel, logPattern); } } } @@ -246,13 +241,13 @@ private String constructLogPattern(String logPathFromConfig) throws SnowflakeSQL logPath = Paths.get(homePath); } - Path path = CreateLogPathSubDirectory(logPath); + Path path = createLogPathSubDirectory(logPath); logPattern = Paths.get(path.toString(), "snowflake_jdbc%u.log").toString(); return logPattern; } - private Path CreateLogPathSubDirectory(Path logPath) throws SnowflakeSQLLoggedException { + private Path createLogPathSubDirectory(Path logPath) throws SnowflakeSQLLoggedException { Path path = Paths.get(logPath.toString(), "jdbc"); if (!Files.exists(path)) { // Create jdbc subfolder @@ -270,7 +265,7 @@ private Path CreateLogPathSubDirectory(Path logPath) throws SnowflakeSQLLoggedEx ErrorCode.INTERNAL_ERROR, String.format( "Un-able to create jdbc subfolder in configfile %s ,%s", - logPath.toString(), ex.getMessage())); + logPath.toString(), ex.getMessage(), ex.getCause())); } } else { // Check if jdbc subfolder has correct permissions @@ -284,10 +279,10 @@ private Path CreateLogPathSubDirectory(Path logPath) throws SnowflakeSQLLoggedEx || folderPermissions.contains(PosixFilePermission.OTHERS_READ) || folderPermissions.contains(PosixFilePermission.OTHERS_EXECUTE)) { logger.info( - String.format( - "Access permission for the logs directory '%s' is currently %s and is potentially " - + "accessible to users other than the owner of the logs directory.", - path.toString(), folderPermissions.toString())); + "Access permission for the logs directory '{}' is currently {} and is potentially " + + "accessible to users other than the owner of the logs directory.", + path.toString(), + folderPermissions.toString()); } } catch (IOException ex) { throw new SnowflakeSQLLoggedException( @@ -295,7 +290,7 @@ private Path CreateLogPathSubDirectory(Path logPath) throws SnowflakeSQLLoggedEx ErrorCode.INTERNAL_ERROR, String.format( "Un-able to get permissions of log directory %s ,%s", - path.toString(), ex.getMessage())); + path.toString(), ex.getMessage(), ex.getCause())); } } } diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java new file mode 100644 index 000000000..2120da668 --- /dev/null +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -0,0 +1,68 @@ +package net.snowflake.client.config; + +import static org.junit.Assert.fail; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.attribute.PosixFilePermissions; +import java.util.HashMap; +import java.util.Map; +import net.snowflake.client.core.Constants; +import org.junit.Test; + +public class SFPermissionsTest { + + Map testConfigFilePermissions = + new HashMap() { + { + put("rwx------", true); + put("rw-------", true); + put("r-x------", true); + put("r--------", true); + put("rwxrwx---", false); + put("rwxrw----", false); + put("rwxr-x---", true); + put("rwxr-----", true); + put("rwx-wx---", false); + put("rwx-w----", false); + put("rwx--x---", true); + put("rwx---rwx", false); + put("rwx---rw-", false); + put("rwx---r-x", true); + put("rwx---r--", true); + put("rwx----wx", false); + put("rwx----w-", false); + put("rwx-----x", true); + } + }; + + @Test + public void testLogDirectoryPermissions() { + if (Constants.getOS() != Constants.OS.WINDOWS) { + Path configFilePath = Paths.get("config.json"); + String configJson = "{\"common\":{\"log_level\":\"debug\",\"log_path\":\"logs\"}}"; + try { + Files.write(configFilePath, configJson.getBytes()); + for (Map.Entry entry : testConfigFilePermissions.entrySet()) { + Files.setPosixFilePermissions( + configFilePath, PosixFilePermissions.fromString(entry.getKey())); + try { + SFClientConfigParser.loadSFClientConfig(configFilePath.toString()); + if (!entry.getValue()) { + fail("testLogDirectoryPermissions failed. Expected exception."); + } + } catch (IOException e) { + if (entry.getValue()) { + fail("testLogDirectoryPermissions failed. Expected pass."); + } + } + } + Files.deleteIfExists(configFilePath); + } catch (IOException e) { + fail("testLogDirectoryPermissions failed"); + } + } + } +} diff --git a/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java b/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java index faa809af4..6425d7ac6 100644 --- a/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java +++ b/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java @@ -1,5 +1,6 @@ package net.snowflake.client.log; +import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetProperty; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -89,4 +90,60 @@ public void testJDK14LoggerWithQuotesInMessage() { logger.debug("Returning column: 12: a: Group b) Hi {Hello 'World' War} cant wait"); JDK14Logger.setLevel(Level.OFF); } + + @Test + public void testJDK14LoggingWithMissingLogPathClientConfig() { + Path configFilePath = Paths.get("config.json"); + String configJson = "{\"common\":{\"log_level\":\"debug\"}}"; + try { + Files.write(configFilePath, configJson.getBytes()); + Properties properties = new Properties(); + properties.put("client_config_file", configFilePath.toString()); + Connection connection = getConnection(properties); + connection.createStatement().executeQuery("select 1"); + + String homePath = systemGetProperty("user.home"); + Path homeLogPath = Paths.get(homePath, "jdbc"); + File file = new File(homeLogPath.toString()); + assertTrue(file.exists()); + + Files.deleteIfExists(configFilePath); + FileUtils.deleteDirectory(new File(homeLogPath.toString())); + } catch (IOException e) { + fail("testJDK14LoggingWithMissingLogPathClientConfig failed"); + } catch (SQLException e) { + fail("testJDK14LoggingWithMissingLogPathClientConfig failed"); + } + } + + @Test + public void testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig() { + String homePath = systemGetProperty("user.home"); + System.clearProperty("user.home"); + + Path configFilePath = Paths.get("config.json"); + String configJson = "{\"common\":{\"log_level\":\"debug\"}}"; + try { + Files.write(configFilePath, configJson.getBytes()); + Properties properties = new Properties(); + properties.put("client_config_file", configFilePath.toString()); + Connection connection = getConnection(properties); + + fail("testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig failed"); + + Files.deleteIfExists(configFilePath); + System.setProperty("user.home", homePath); + } catch (IOException e) { + fail("testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig failed"); + } catch (SQLException e) { + // Succeed + } finally { + try { + System.setProperty("user.home", homePath); + Files.deleteIfExists(configFilePath); + } catch (IOException e) { + fail("testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig failed"); + } + } + } } From 0d8958d26792ebd0013b3360250f427c3768f941 Mon Sep 17 00:00:00 2001 From: norrislee Date: Thu, 2 May 2024 15:58:27 -0700 Subject: [PATCH 05/19] Parameterize test, small minor refactorings --- .../client/config/SFClientConfig.java | 2 +- .../client/config/SFClientConfigParser.java | 3 +- .../jdbc/DefaultSFConnectionHandler.java | 82 ++++++------ .../net/snowflake/client/RunningOnWin.java | 9 ++ .../client/config/SFPermissionsTest.java | 123 +++++++++++------- .../log/JDK14LoggerWithClientLatestIT.java | 22 ++-- 6 files changed, 145 insertions(+), 96 deletions(-) create mode 100644 src/test/java/net/snowflake/client/RunningOnWin.java diff --git a/src/main/java/net/snowflake/client/config/SFClientConfig.java b/src/main/java/net/snowflake/client/config/SFClientConfig.java index 0ed7c81ae..47b679d74 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfig.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfig.java @@ -85,7 +85,7 @@ public void setLogPath(String logPath) { this.logPath = logPath; } - protected Map getUnknownKeys() { + Map getUnknownKeys() { return unknownKeys; } diff --git a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java index 4a3614bc1..6f4f20af4 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java @@ -29,8 +29,7 @@ public class SFClientConfigParser { * connection property. 2. Environment variable: SF_CLIENT_CONFIG_FILE containing full path to * sf_client_config file. 3. Searches for default config file name(sf_client_config.json under the * driver directory from where the driver gets loaded. 4. Searches for default config file - * name(sf_client_config.json) under user home directory 5. Searches for default config file - * name(sf_client_config.json) under tmp directory. + * name(sf_client_config.json) under user home directory. * * @param configFilePath SF_CLIENT_CONFIG_FILE parameter read from connection URL or connection * properties diff --git a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java index 32a02f4bd..708d29b48 100644 --- a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java +++ b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java @@ -250,51 +250,57 @@ private String constructLogPattern(String logPathFromConfig) throws SnowflakeSQL private Path createLogPathSubDirectory(Path logPath) throws SnowflakeSQLLoggedException { Path path = Paths.get(logPath.toString(), "jdbc"); if (!Files.exists(path)) { - // Create jdbc subfolder - try { - if (Constants.getOS() == Constants.OS.WINDOWS) { - Files.createDirectories(path); - } else { - Files.createDirectories( - path, - PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"))); - } - } catch (IOException ex) { - throw new SnowflakeSQLLoggedException( - sfSession, - ErrorCode.INTERNAL_ERROR, - String.format( - "Un-able to create jdbc subfolder in configfile %s ,%s", - logPath.toString(), ex.getMessage(), ex.getCause())); - } + createLogFolder(path); } else { - // Check if jdbc subfolder has correct permissions - if (Constants.getOS() != Constants.OS.WINDOWS) { - try { - Set folderPermissions = Files.getPosixFilePermissions(path); - if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE) - || folderPermissions.contains(PosixFilePermission.GROUP_READ) - || folderPermissions.contains(PosixFilePermission.GROUP_EXECUTE) - || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE) - || folderPermissions.contains(PosixFilePermission.OTHERS_READ) - || folderPermissions.contains(PosixFilePermission.OTHERS_EXECUTE)) { - logger.info( - "Access permission for the logs directory '{}' is currently {} and is potentially " - + "accessible to users other than the owner of the logs directory.", - path.toString(), - folderPermissions.toString()); - } - } catch (IOException ex) { - throw new SnowflakeSQLLoggedException( + checkLogFolderPermissions(path); + } + return path; + } + + private void createLogFolder(Path path) throws SnowflakeSQLLoggedException{ + try { + if (Constants.getOS() == Constants.OS.WINDOWS) { + Files.createDirectories(path); + } else { + Files.createDirectories( + path, + PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"))); + } + } catch (IOException ex) { + throw new SnowflakeSQLLoggedException( sfSession, ErrorCode.INTERNAL_ERROR, String.format( - "Un-able to get permissions of log directory %s ,%s", - path.toString(), ex.getMessage(), ex.getCause())); + "Unable to create jdbc subfolder in configfile %s ,%s", + path.toString(), ex.getMessage(), ex.getCause())); + } + } + + private void checkLogFolderPermissions(Path path) throws SnowflakeSQLLoggedException { + if (Constants.getOS() != Constants.OS.WINDOWS) { + try { + Set folderPermissions = Files.getPosixFilePermissions(path); + if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE) + || folderPermissions.contains(PosixFilePermission.GROUP_READ) + || folderPermissions.contains(PosixFilePermission.GROUP_EXECUTE) + || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE) + || folderPermissions.contains(PosixFilePermission.OTHERS_READ) + || folderPermissions.contains(PosixFilePermission.OTHERS_EXECUTE)) { + logger.info( + "Access permission for the logs directory '{}' is currently {} and is potentially " + + "accessible to users other than the owner of the logs directory.", + path.toString(), + folderPermissions.toString()); } + } catch (IOException ex) { + throw new SnowflakeSQLLoggedException( + sfSession, + ErrorCode.INTERNAL_ERROR, + String.format( + "Un-able to get permissions of log directory %s ,%s", + path.toString(), ex.getMessage(), ex.getCause())); } } - return path; } private void initSessionProperties(SnowflakeConnectString conStr, String appID, String appVersion) diff --git a/src/test/java/net/snowflake/client/RunningOnWin.java b/src/test/java/net/snowflake/client/RunningOnWin.java new file mode 100644 index 000000000..22d0d1bbf --- /dev/null +++ b/src/test/java/net/snowflake/client/RunningOnWin.java @@ -0,0 +1,9 @@ +package net.snowflake.client; + +import net.snowflake.client.core.Constants; + +public class RunningOnWin implements ConditionalIgnoreRule.IgnoreCondition { + public boolean isSatisfied() { + return Constants.getOS() == Constants.OS.WINDOWS; + } +} diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java index 2120da668..05515a3a0 100644 --- a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -9,60 +9,91 @@ import java.nio.file.attribute.PosixFilePermissions; import java.util.HashMap; import java.util.Map; -import net.snowflake.client.core.Constants; +import java.util.Set; + +import net.snowflake.client.ConditionalIgnoreRule; +import net.snowflake.client.RunningOnWin; +import org.junit.After; +import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +@RunWith(Parameterized.class) public class SFPermissionsTest { - Map testConfigFilePermissions = - new HashMap() { - { - put("rwx------", true); - put("rw-------", true); - put("r-x------", true); - put("r--------", true); - put("rwxrwx---", false); - put("rwxrw----", false); - put("rwxr-x---", true); - put("rwxr-----", true); - put("rwx-wx---", false); - put("rwx-w----", false); - put("rwx--x---", true); - put("rwx---rwx", false); - put("rwx---rw-", false); - put("rwx---r-x", true); - put("rwx---r--", true); - put("rwx----wx", false); - put("rwx----w-", false); - put("rwx-----x", true); - } - }; + @Parameterized.Parameters(name = "permission={0}") + public static Set> data() { + Map testConfigFilePermissions = new HashMap() { + { + put("rwx------", true); + put("rw-------", true); + put("r-x------", true); + put("r--------", true); + put("rwxrwx---", false); + put("rwxrw----", false); + put("rwxr-x---", true); + put("rwxr-----", true); + put("rwx-wx---", false); + put("rwx-w----", false); + put("rwx--x---", true); + put("rwx---rwx", false); + put("rwx---rw-", false); + put("rwx---r-x", true); + put("rwx---r--", true); + put("rwx----wx", false); + put("rwx----w-", false); + put("rwx-----x", true); + } + }; + return testConfigFilePermissions.entrySet(); + } + + Path configFilePath = Paths.get("config.json"); + String configJson = "{\"common\":{\"log_level\":\"debug\",\"log_path\":\"logs\"}}"; + String permission; + Boolean isSucceed; + + + public SFPermissionsTest(Map.Entry permission) { + this.permission = permission.getKey(); + this.isSucceed = permission.getValue(); + } + @Before + public void createConfigFile() { + try { + Files.write(configFilePath, configJson.getBytes()); + } catch (IOException e) { + fail("testLogDirectoryPermissions failed setup"); + } + } + + @After + public void cleanupConfigFile() { + try { + Files.deleteIfExists(configFilePath); + } catch (IOException e) { + fail("testLogDirectoryPermissions failed cleanup"); + } + } @Test + @ConditionalIgnoreRule.ConditionalIgnore(condition = RunningOnWin.class) public void testLogDirectoryPermissions() { - if (Constants.getOS() != Constants.OS.WINDOWS) { - Path configFilePath = Paths.get("config.json"); - String configJson = "{\"common\":{\"log_level\":\"debug\",\"log_path\":\"logs\"}}"; - try { - Files.write(configFilePath, configJson.getBytes()); - for (Map.Entry entry : testConfigFilePermissions.entrySet()) { - Files.setPosixFilePermissions( - configFilePath, PosixFilePermissions.fromString(entry.getKey())); - try { - SFClientConfigParser.loadSFClientConfig(configFilePath.toString()); - if (!entry.getValue()) { - fail("testLogDirectoryPermissions failed. Expected exception."); - } - } catch (IOException e) { - if (entry.getValue()) { - fail("testLogDirectoryPermissions failed. Expected pass."); - } - } - } - Files.deleteIfExists(configFilePath); - } catch (IOException e) { - fail("testLogDirectoryPermissions failed"); + // Don't run on Windows + try { + Files.setPosixFilePermissions( + configFilePath, PosixFilePermissions.fromString(permission)); + SFClientConfigParser.loadSFClientConfig(configFilePath.toString()); + if (!isSucceed) { + fail("testLogDirectoryPermissions failed. Expected exception."); + } + } catch (IOException e) { + if (isSucceed) { + fail("testLogDirectoryPermissions failed. Expected pass."); } + } catch (Exception e) { + fail("testLogDirectoryPermissions failed. Unknown exception."); } } } diff --git a/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java b/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java index 6425d7ac6..09f77b7e6 100644 --- a/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java +++ b/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java @@ -22,6 +22,8 @@ public class JDK14LoggerWithClientLatestIT extends AbstractDriverIT { + String homePath = systemGetProperty("user.home"); + @Test public void testJDK14LoggingWithClientConfig() { Path configFilePath = Paths.get("config.json"); @@ -95,6 +97,8 @@ public void testJDK14LoggerWithQuotesInMessage() { public void testJDK14LoggingWithMissingLogPathClientConfig() { Path configFilePath = Paths.get("config.json"); String configJson = "{\"common\":{\"log_level\":\"debug\"}}"; + + Path homeLogPath = Paths.get(homePath, "jdbc"); try { Files.write(configFilePath, configJson.getBytes()); Properties properties = new Properties(); @@ -102,23 +106,26 @@ public void testJDK14LoggingWithMissingLogPathClientConfig() { Connection connection = getConnection(properties); connection.createStatement().executeQuery("select 1"); - String homePath = systemGetProperty("user.home"); - Path homeLogPath = Paths.get(homePath, "jdbc"); File file = new File(homeLogPath.toString()); assertTrue(file.exists()); - - Files.deleteIfExists(configFilePath); - FileUtils.deleteDirectory(new File(homeLogPath.toString())); } catch (IOException e) { fail("testJDK14LoggingWithMissingLogPathClientConfig failed"); } catch (SQLException e) { fail("testJDK14LoggingWithMissingLogPathClientConfig failed"); + } catch (Exception e) { + fail("testJDK14LoggingWithMissingLogPathClientConfig failed"); + } finally { + try { + Files.deleteIfExists(configFilePath); + FileUtils.deleteDirectory(new File(homeLogPath.toString())); + } catch (IOException e) { + fail("testJDK14LoggingWithMissingLogPathClientConfig failed"); + } } } @Test public void testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig() { - String homePath = systemGetProperty("user.home"); System.clearProperty("user.home"); Path configFilePath = Paths.get("config.json"); @@ -130,9 +137,6 @@ public void testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig() { Connection connection = getConnection(properties); fail("testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig failed"); - - Files.deleteIfExists(configFilePath); - System.setProperty("user.home", homePath); } catch (IOException e) { fail("testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig failed"); } catch (SQLException e) { From c6bca0039f515f23488ecb13aafe844de19c8c2a Mon Sep 17 00:00:00 2001 From: norrislee Date: Thu, 2 May 2024 16:02:07 -0700 Subject: [PATCH 06/19] Fix formatting --- .../jdbc/DefaultSFConnectionHandler.java | 44 ++++++++-------- .../net/snowflake/client/RunningOnWin.java | 6 +-- .../client/config/SFPermissionsTest.java | 51 +++++++++---------- 3 files changed, 50 insertions(+), 51 deletions(-) diff --git a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java index 708d29b48..8030f786a 100644 --- a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java +++ b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java @@ -257,22 +257,22 @@ private Path createLogPathSubDirectory(Path logPath) throws SnowflakeSQLLoggedEx return path; } - private void createLogFolder(Path path) throws SnowflakeSQLLoggedException{ + private void createLogFolder(Path path) throws SnowflakeSQLLoggedException { try { if (Constants.getOS() == Constants.OS.WINDOWS) { Files.createDirectories(path); } else { Files.createDirectories( - path, - PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"))); + path, + PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"))); } } catch (IOException ex) { throw new SnowflakeSQLLoggedException( - sfSession, - ErrorCode.INTERNAL_ERROR, - String.format( - "Unable to create jdbc subfolder in configfile %s ,%s", - path.toString(), ex.getMessage(), ex.getCause())); + sfSession, + ErrorCode.INTERNAL_ERROR, + String.format( + "Unable to create jdbc subfolder in configfile %s ,%s", + path.toString(), ex.getMessage(), ex.getCause())); } } @@ -281,24 +281,24 @@ private void checkLogFolderPermissions(Path path) throws SnowflakeSQLLoggedExcep try { Set folderPermissions = Files.getPosixFilePermissions(path); if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE) - || folderPermissions.contains(PosixFilePermission.GROUP_READ) - || folderPermissions.contains(PosixFilePermission.GROUP_EXECUTE) - || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE) - || folderPermissions.contains(PosixFilePermission.OTHERS_READ) - || folderPermissions.contains(PosixFilePermission.OTHERS_EXECUTE)) { + || folderPermissions.contains(PosixFilePermission.GROUP_READ) + || folderPermissions.contains(PosixFilePermission.GROUP_EXECUTE) + || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE) + || folderPermissions.contains(PosixFilePermission.OTHERS_READ) + || folderPermissions.contains(PosixFilePermission.OTHERS_EXECUTE)) { logger.info( - "Access permission for the logs directory '{}' is currently {} and is potentially " - + "accessible to users other than the owner of the logs directory.", - path.toString(), - folderPermissions.toString()); + "Access permission for the logs directory '{}' is currently {} and is potentially " + + "accessible to users other than the owner of the logs directory.", + path.toString(), + folderPermissions.toString()); } } catch (IOException ex) { throw new SnowflakeSQLLoggedException( - sfSession, - ErrorCode.INTERNAL_ERROR, - String.format( - "Un-able to get permissions of log directory %s ,%s", - path.toString(), ex.getMessage(), ex.getCause())); + sfSession, + ErrorCode.INTERNAL_ERROR, + String.format( + "Un-able to get permissions of log directory %s ,%s", + path.toString(), ex.getMessage(), ex.getCause())); } } } diff --git a/src/test/java/net/snowflake/client/RunningOnWin.java b/src/test/java/net/snowflake/client/RunningOnWin.java index 22d0d1bbf..025ab1e04 100644 --- a/src/test/java/net/snowflake/client/RunningOnWin.java +++ b/src/test/java/net/snowflake/client/RunningOnWin.java @@ -3,7 +3,7 @@ import net.snowflake.client.core.Constants; public class RunningOnWin implements ConditionalIgnoreRule.IgnoreCondition { - public boolean isSatisfied() { - return Constants.getOS() == Constants.OS.WINDOWS; - } + public boolean isSatisfied() { + return Constants.getOS() == Constants.OS.WINDOWS; + } } diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java index 05515a3a0..d593c0858 100644 --- a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -10,7 +10,6 @@ import java.util.HashMap; import java.util.Map; import java.util.Set; - import net.snowflake.client.ConditionalIgnoreRule; import net.snowflake.client.RunningOnWin; import org.junit.After; @@ -24,28 +23,29 @@ public class SFPermissionsTest { @Parameterized.Parameters(name = "permission={0}") public static Set> data() { - Map testConfigFilePermissions = new HashMap() { - { - put("rwx------", true); - put("rw-------", true); - put("r-x------", true); - put("r--------", true); - put("rwxrwx---", false); - put("rwxrw----", false); - put("rwxr-x---", true); - put("rwxr-----", true); - put("rwx-wx---", false); - put("rwx-w----", false); - put("rwx--x---", true); - put("rwx---rwx", false); - put("rwx---rw-", false); - put("rwx---r-x", true); - put("rwx---r--", true); - put("rwx----wx", false); - put("rwx----w-", false); - put("rwx-----x", true); - } - }; + Map testConfigFilePermissions = + new HashMap() { + { + put("rwx------", true); + put("rw-------", true); + put("r-x------", true); + put("r--------", true); + put("rwxrwx---", false); + put("rwxrw----", false); + put("rwxr-x---", true); + put("rwxr-----", true); + put("rwx-wx---", false); + put("rwx-w----", false); + put("rwx--x---", true); + put("rwx---rwx", false); + put("rwx---rw-", false); + put("rwx---r-x", true); + put("rwx---r--", true); + put("rwx----wx", false); + put("rwx----w-", false); + put("rwx-----x", true); + } + }; return testConfigFilePermissions.entrySet(); } @@ -54,11 +54,11 @@ public static Set> data() { String permission; Boolean isSucceed; - public SFPermissionsTest(Map.Entry permission) { this.permission = permission.getKey(); this.isSucceed = permission.getValue(); } + @Before public void createConfigFile() { try { @@ -82,8 +82,7 @@ public void cleanupConfigFile() { public void testLogDirectoryPermissions() { // Don't run on Windows try { - Files.setPosixFilePermissions( - configFilePath, PosixFilePermissions.fromString(permission)); + Files.setPosixFilePermissions(configFilePath, PosixFilePermissions.fromString(permission)); SFClientConfigParser.loadSFClientConfig(configFilePath.toString()); if (!isSucceed) { fail("testLogDirectoryPermissions failed. Expected exception."); From 701bf67f7c3b7e6cea4899d7d4af14cf5caeb18e Mon Sep 17 00:00:00 2001 From: norrislee Date: Mon, 27 May 2024 17:32:25 -0700 Subject: [PATCH 07/19] Fix error message, remove unnecessary catch in tests --- .../jdbc/DefaultSFConnectionHandler.java | 2 +- .../client/config/SFPermissionsTest.java | 34 +++++-------------- .../log/JDK14LoggerWithClientLatestIT.java | 25 +++++--------- 3 files changed, 18 insertions(+), 43 deletions(-) diff --git a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java index 8030f786a..9e7e59038 100644 --- a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java +++ b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java @@ -223,7 +223,7 @@ private String constructLogPattern(String logPathFromConfig) throws SnowflakeSQL sfSession, ErrorCode.INTERNAL_ERROR, String.format( - "Un-able to create log path mentioned in configfile %s ,%s", + "Unable to create log path mentioned in configfile %s ,%s", logPathFromConfig, ex.getMessage())); } } diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java index d593c0858..5cb887bb5 100644 --- a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -60,39 +60,23 @@ public SFPermissionsTest(Map.Entry permission) { } @Before - public void createConfigFile() { - try { - Files.write(configFilePath, configJson.getBytes()); - } catch (IOException e) { - fail("testLogDirectoryPermissions failed setup"); - } + public void createConfigFile() throws IOException { + Files.write(configFilePath, configJson.getBytes()); } @After - public void cleanupConfigFile() { - try { - Files.deleteIfExists(configFilePath); - } catch (IOException e) { - fail("testLogDirectoryPermissions failed cleanup"); - } + public void cleanupConfigFile() throws IOException { + Files.deleteIfExists(configFilePath); } @Test @ConditionalIgnoreRule.ConditionalIgnore(condition = RunningOnWin.class) - public void testLogDirectoryPermissions() { + public void testLogDirectoryPermissions() throws IOException { // Don't run on Windows - try { - Files.setPosixFilePermissions(configFilePath, PosixFilePermissions.fromString(permission)); - SFClientConfigParser.loadSFClientConfig(configFilePath.toString()); - if (!isSucceed) { - fail("testLogDirectoryPermissions failed. Expected exception."); - } - } catch (IOException e) { - if (isSucceed) { - fail("testLogDirectoryPermissions failed. Expected pass."); - } - } catch (Exception e) { - fail("testLogDirectoryPermissions failed. Unknown exception."); + Files.setPosixFilePermissions(configFilePath, PosixFilePermissions.fromString(permission)); + SFClientConfigParser.loadSFClientConfig(configFilePath.toString()); + if (!isSucceed) { + fail("testLogDirectoryPermissions failed. Expected exception."); } } } diff --git a/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java b/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java index 6cb9376f4..ef510a0ff 100644 --- a/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java +++ b/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java @@ -18,6 +18,7 @@ import java.util.Properties; import java.util.logging.Level; import net.snowflake.client.AbstractDriverIT; +import net.snowflake.client.jdbc.SnowflakeSQLLoggedException; import org.apache.commons.io.FileUtils; import org.junit.Test; @@ -98,7 +99,7 @@ public void testJDK14LoggerWithQuotesInMessage() { } @Test - public void testJDK14LoggingWithMissingLogPathClientConfig() { + public void testJDK14LoggingWithMissingLogPathClientConfig() throws IOException { Path configFilePath = Paths.get("config.json"); String configJson = "{\"common\":{\"log_level\":\"debug\"}}"; @@ -119,17 +120,13 @@ public void testJDK14LoggingWithMissingLogPathClientConfig() { } catch (Exception e) { fail("testJDK14LoggingWithMissingLogPathClientConfig failed"); } finally { - try { - Files.deleteIfExists(configFilePath); - FileUtils.deleteDirectory(new File(homeLogPath.toString())); - } catch (IOException e) { - fail("testJDK14LoggingWithMissingLogPathClientConfig failed"); - } + Files.deleteIfExists(configFilePath); + FileUtils.deleteDirectory(new File(homeLogPath.toString())); } } @Test - public void testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig() { + public void testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig() throws Exception { System.clearProperty("user.home"); Path configFilePath = Paths.get("config.json"); @@ -141,17 +138,11 @@ public void testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig() { Connection connection = getConnection(properties); fail("testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig failed"); - } catch (IOException e) { - fail("testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig failed"); - } catch (SQLException e) { + } catch (SnowflakeSQLLoggedException e) { // Succeed } finally { - try { - System.setProperty("user.home", homePath); - Files.deleteIfExists(configFilePath); - } catch (IOException e) { - fail("testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig failed"); - } + System.setProperty("user.home", homePath); + Files.deleteIfExists(configFilePath); } } } From 91219336b3d69a39d802e8a9047cd764b0ad4bca Mon Sep 17 00:00:00 2001 From: norrislee Date: Mon, 27 May 2024 17:53:31 -0700 Subject: [PATCH 08/19] Fix test IOException issue --- .../client/config/SFPermissionsTest.java | 58 ++++++++++--------- 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java index 5cb887bb5..35e2aff5a 100644 --- a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -24,28 +24,28 @@ public class SFPermissionsTest { @Parameterized.Parameters(name = "permission={0}") public static Set> data() { Map testConfigFilePermissions = - new HashMap() { - { - put("rwx------", true); - put("rw-------", true); - put("r-x------", true); - put("r--------", true); - put("rwxrwx---", false); - put("rwxrw----", false); - put("rwxr-x---", true); - put("rwxr-----", true); - put("rwx-wx---", false); - put("rwx-w----", false); - put("rwx--x---", true); - put("rwx---rwx", false); - put("rwx---rw-", false); - put("rwx---r-x", true); - put("rwx---r--", true); - put("rwx----wx", false); - put("rwx----w-", false); - put("rwx-----x", true); - } - }; + new HashMap() { + { + put("rwx------", true); + put("rw-------", true); + put("r-x------", true); + put("r--------", true); + put("rwxrwx---", false); + put("rwxrw----", false); + put("rwxr-x---", true); + put("rwxr-----", true); + put("rwx-wx---", false); + put("rwx-w----", false); + put("rwx--x---", true); + put("rwx---rwx", false); + put("rwx---rw-", false); + put("rwx---r-x", true); + put("rwx---r--", true); + put("rwx----wx", false); + put("rwx----w-", false); + put("rwx-----x", true); + } + }; return testConfigFilePermissions.entrySet(); } @@ -73,10 +73,16 @@ public void cleanupConfigFile() throws IOException { @ConditionalIgnoreRule.ConditionalIgnore(condition = RunningOnWin.class) public void testLogDirectoryPermissions() throws IOException { // Don't run on Windows - Files.setPosixFilePermissions(configFilePath, PosixFilePermissions.fromString(permission)); - SFClientConfigParser.loadSFClientConfig(configFilePath.toString()); - if (!isSucceed) { - fail("testLogDirectoryPermissions failed. Expected exception."); + try { + Files.setPosixFilePermissions(configFilePath, PosixFilePermissions.fromString(permission)); + SFClientConfigParser.loadSFClientConfig(configFilePath.toString()); + if (!isSucceed) { + fail("testLogDirectoryPermissions failed. Expected exception."); + } + } catch (IOException e) { + if (isSucceed) { + fail("testLogDirectoryPermissions failed. Expected pass."); + } } } } From ccf89291d435939b2013edfd70533999fd072c3b Mon Sep 17 00:00:00 2001 From: norrislee Date: Mon, 27 May 2024 18:04:59 -0700 Subject: [PATCH 09/19] Fix formatting --- .../client/config/SFPermissionsTest.java | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java index 35e2aff5a..3a7e83cea 100644 --- a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -24,28 +24,28 @@ public class SFPermissionsTest { @Parameterized.Parameters(name = "permission={0}") public static Set> data() { Map testConfigFilePermissions = - new HashMap() { - { - put("rwx------", true); - put("rw-------", true); - put("r-x------", true); - put("r--------", true); - put("rwxrwx---", false); - put("rwxrw----", false); - put("rwxr-x---", true); - put("rwxr-----", true); - put("rwx-wx---", false); - put("rwx-w----", false); - put("rwx--x---", true); - put("rwx---rwx", false); - put("rwx---rw-", false); - put("rwx---r-x", true); - put("rwx---r--", true); - put("rwx----wx", false); - put("rwx----w-", false); - put("rwx-----x", true); - } - }; + new HashMap() { + { + put("rwx------", true); + put("rw-------", true); + put("r-x------", true); + put("r--------", true); + put("rwxrwx---", false); + put("rwxrw----", false); + put("rwxr-x---", true); + put("rwxr-----", true); + put("rwx-wx---", false); + put("rwx-w----", false); + put("rwx--x---", true); + put("rwx---rwx", false); + put("rwx---rw-", false); + put("rwx---r-x", true); + put("rwx---r--", true); + put("rwx----wx", false); + put("rwx----w-", false); + put("rwx-----x", true); + } + }; return testConfigFilePermissions.entrySet(); } From 74a6fcb8e1f3f0bd5d72903556a0a5ef1929f0a9 Mon Sep 17 00:00:00 2001 From: norrislee Date: Mon, 3 Jun 2024 14:47:56 -0700 Subject: [PATCH 10/19] add try with resources, fix log messages --- .../jdbc/DefaultSFConnectionHandler.java | 4 +- .../log/JDK14LoggerWithClientLatestIT.java | 43 +++++++++---------- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java index 9e7e59038..6bb62c82f 100644 --- a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java +++ b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java @@ -286,7 +286,7 @@ private void checkLogFolderPermissions(Path path) throws SnowflakeSQLLoggedExcep || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE) || folderPermissions.contains(PosixFilePermission.OTHERS_READ) || folderPermissions.contains(PosixFilePermission.OTHERS_EXECUTE)) { - logger.info( + logger.warn( "Access permission for the logs directory '{}' is currently {} and is potentially " + "accessible to users other than the owner of the logs directory.", path.toString(), @@ -297,7 +297,7 @@ private void checkLogFolderPermissions(Path path) throws SnowflakeSQLLoggedExcep sfSession, ErrorCode.INTERNAL_ERROR, String.format( - "Un-able to get permissions of log directory %s ,%s", + "Unable to get permissions of log directory %s ,%s", path.toString(), ex.getMessage(), ex.getCause())); } } diff --git a/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java b/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java index ef510a0ff..232da8451 100644 --- a/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java +++ b/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java @@ -99,29 +99,26 @@ public void testJDK14LoggerWithQuotesInMessage() { } @Test - public void testJDK14LoggingWithMissingLogPathClientConfig() throws IOException { + public void testJDK14LoggingWithMissingLogPathClientConfig() throws Exception { Path configFilePath = Paths.get("config.json"); String configJson = "{\"common\":{\"log_level\":\"debug\"}}"; Path homeLogPath = Paths.get(homePath, "jdbc"); - try { - Files.write(configFilePath, configJson.getBytes()); - Properties properties = new Properties(); - properties.put("client_config_file", configFilePath.toString()); - Connection connection = getConnection(properties); - connection.createStatement().executeQuery("select 1"); + Files.write(configFilePath, configJson.getBytes()); + Properties properties = new Properties(); + properties.put("client_config_file", configFilePath.toString()); + try (Connection connection = getConnection(properties); + Statement statement = connection.createStatement()) { + try { + statement.executeQuery("select 1"); - File file = new File(homeLogPath.toString()); - assertTrue(file.exists()); - } catch (IOException e) { - fail("testJDK14LoggingWithMissingLogPathClientConfig failed"); - } catch (SQLException e) { - fail("testJDK14LoggingWithMissingLogPathClientConfig failed"); - } catch (Exception e) { - fail("testJDK14LoggingWithMissingLogPathClientConfig failed"); - } finally { - Files.deleteIfExists(configFilePath); - FileUtils.deleteDirectory(new File(homeLogPath.toString())); + File file = new File(homeLogPath.toString()); + assertTrue(file.exists()); + + } finally { + Files.deleteIfExists(configFilePath); + FileUtils.deleteDirectory(new File(homeLogPath.toString())); + } } } @@ -131,11 +128,11 @@ public void testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig() throws Exc Path configFilePath = Paths.get("config.json"); String configJson = "{\"common\":{\"log_level\":\"debug\"}}"; - try { - Files.write(configFilePath, configJson.getBytes()); - Properties properties = new Properties(); - properties.put("client_config_file", configFilePath.toString()); - Connection connection = getConnection(properties); + Files.write(configFilePath, configJson.getBytes()); + Properties properties = new Properties(); + properties.put("client_config_file", configFilePath.toString()); + try (Connection connection = getConnection(properties); + Statement statement = connection.createStatement()) { fail("testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig failed"); } catch (SnowflakeSQLLoggedException e) { From f6906980aa1785ca11eb2540f859f355c75cbe90 Mon Sep 17 00:00:00 2001 From: norrislee Date: Thu, 13 Jun 2024 14:46:49 -0700 Subject: [PATCH 11/19] Fix styling --- .../net/snowflake/client/config/SFClientConfigParser.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java index c43417848..69f48386b 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java @@ -9,7 +9,6 @@ import java.nio.file.Files; import java.nio.file.Paths; import java.nio.file.attribute.PosixFilePermission; -import java.util.Map; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -84,11 +83,11 @@ public static SFClientConfig loadSFClientConfig(String configFilePath) throws IO File configFile = new File(derivedConfigFilePath); ObjectMapper objectMapper = new ObjectMapper(); SFClientConfig clientConfig = objectMapper.readValue(configFile, SFClientConfig.class); - logger.info( + logger.info( "Reading values logLevel {} and logPath {} from client configuration", clientConfig.getCommonProps().getLogLevel(), clientConfig.getCommonProps().getLogPath()); - + Set unknownParams = clientConfig.getUnknownParamKeys(); if (!unknownParams.isEmpty()) { for (String unknownParam : unknownParams) { From e68e66fcf0b6acaea9927361b92a606848c80b0d Mon Sep 17 00:00:00 2001 From: norrislee Date: Thu, 13 Jun 2024 17:46:35 -0700 Subject: [PATCH 12/19] Remove unnecessary code --- .../java/net/snowflake/client/config/SFClientConfig.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/main/java/net/snowflake/client/config/SFClientConfig.java b/src/main/java/net/snowflake/client/config/SFClientConfig.java index 6dd29619c..a11071b1f 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfig.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfig.java @@ -81,8 +81,6 @@ public static class CommonProps { @JsonProperty("log_path") private String logPath; - @JsonAnySetter private Map unknownKeys = new LinkedHashMap<>(); - public CommonProps() {} public void CommonProps(String logLevel, String logPath) { @@ -106,10 +104,6 @@ public void setLogPath(String logPath) { this.logPath = logPath; } - Map getUnknownKeys() { - return unknownKeys; - } - @Override public boolean equals(Object o) { if (this == o) { From acdf254cdc6e2581fa08ae01fdd95b3492ed5381 Mon Sep 17 00:00:00 2001 From: norrislee Date: Tue, 25 Jun 2024 09:53:44 -0700 Subject: [PATCH 13/19] On config file permission error, change throw to warn instead --- .../java/net/snowflake/client/config/SFClientConfigParser.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java index 69f48386b..51196a590 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java @@ -77,7 +77,8 @@ public static SFClientConfig loadSFClientConfig(String configFilePath) throws IO String.format( "Error due to other users having permission to modify the config file: %s", derivedConfigFilePath); - throw new IOException(error); + // TODO: SNOW-1503722 to change warning log to throw an error instead + logger.warn(error); } } File configFile = new File(derivedConfigFilePath); From 243b2326bb482aa95d938fa88ad95ede3f3f90ba Mon Sep 17 00:00:00 2001 From: norrislee Date: Tue, 25 Jun 2024 16:26:47 -0700 Subject: [PATCH 14/19] Update permission test since we no longer through exception --- .../client/config/SFClientConfigParser.java | 40 +++++++++++++------ .../client/config/SFPermissionsTest.java | 15 +++---- 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java index 51196a590..e6bdeceb5 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java @@ -67,20 +67,10 @@ public static SFClientConfig loadSFClientConfig(String configFilePath) throws IO } if (derivedConfigFilePath != null) { try { - if (Constants.getOS() != Constants.OS.WINDOWS) { - // Check permissions of config file - Set folderPermissions = - Files.getPosixFilePermissions(Paths.get(derivedConfigFilePath)); - if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE) - || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE)) { - String error = - String.format( - "Error due to other users having permission to modify the config file: %s", - derivedConfigFilePath); - // TODO: SNOW-1503722 to change warning log to throw an error instead - logger.warn(error); - } + if (!checkConfigFilePermissions(derivedConfigFilePath)) { + return null; } + File configFile = new File(derivedConfigFilePath); ObjectMapper objectMapper = new ObjectMapper(); SFClientConfig clientConfig = objectMapper.readValue(configFile, SFClientConfig.class); @@ -134,6 +124,30 @@ && systemGetProperty("os.name").toLowerCase().startsWith("windows")) { } } + public static Boolean checkConfigFilePermissions(String derivedConfigFilePath) { + try { + if (Constants.getOS() != Constants.OS.WINDOWS) { + // Check permissions of config file + Set folderPermissions = + Files.getPosixFilePermissions(Paths.get(derivedConfigFilePath)); + if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE) + || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE)) { + String error = + String.format( + "Error due to other users having permission to modify the config file: %s", + derivedConfigFilePath); + // TODO: SNOW-1503722 to change warning log to throw an error instead + logger.warn(error); + return false; + } + } + } catch (IOException e) { + logger.warn(e.getMessage()); + return false; + } + return true; + } + static String convertToWindowsPath(String filePath) { // Find the Windows file path pattern: ex) C:\ or D:\ Pattern windowsFilePattern = Pattern.compile("[C-Z]:[\\\\/]"); diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java index 3a7e83cea..a3d62647c 100644 --- a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -72,17 +72,12 @@ public void cleanupConfigFile() throws IOException { @Test @ConditionalIgnoreRule.ConditionalIgnore(condition = RunningOnWin.class) public void testLogDirectoryPermissions() throws IOException { + // TODO: SNOW-1503722 Change to check for thrown exceptions // Don't run on Windows - try { - Files.setPosixFilePermissions(configFilePath, PosixFilePermissions.fromString(permission)); - SFClientConfigParser.loadSFClientConfig(configFilePath.toString()); - if (!isSucceed) { - fail("testLogDirectoryPermissions failed. Expected exception."); - } - } catch (IOException e) { - if (isSucceed) { - fail("testLogDirectoryPermissions failed. Expected pass."); - } + Files.setPosixFilePermissions(configFilePath, PosixFilePermissions.fromString(permission)); + Boolean result = SFClientConfigParser.checkConfigFilePermissions(configFilePath.toString()); + if (isSucceed != result) { + fail("testLogDirectoryPermissions failed. Expected " + isSucceed); } } } From d36b9bebe2414393ee46816b7080529b91199cfe Mon Sep 17 00:00:00 2001 From: norrislee Date: Tue, 25 Jun 2024 17:11:09 -0700 Subject: [PATCH 15/19] Throw exception when its not a permission reason --- .../net/snowflake/client/config/SFClientConfigParser.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java index e6bdeceb5..56c414507 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java @@ -124,7 +124,8 @@ && systemGetProperty("os.name").toLowerCase().startsWith("windows")) { } } - public static Boolean checkConfigFilePermissions(String derivedConfigFilePath) { + public static Boolean checkConfigFilePermissions(String derivedConfigFilePath) + throws IOException { try { if (Constants.getOS() != Constants.OS.WINDOWS) { // Check permissions of config file @@ -142,8 +143,7 @@ public static Boolean checkConfigFilePermissions(String derivedConfigFilePath) { } } } catch (IOException e) { - logger.warn(e.getMessage()); - return false; + throw e; } return true; } From 2a47f625e0138ad5d0b1fa445b970cb2d71e1a2a Mon Sep 17 00:00:00 2001 From: norrislee Date: Wed, 26 Jun 2024 23:24:51 -0700 Subject: [PATCH 16/19] Move permissions function to a separate package scope function. Fix test case to only run on posix --- .../client/config/SFClientConfigParser.java | 21 ++++++++++--------- .../client/config/SFPermissionsTest.java | 10 +++++---- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java index 56c414507..b96652ca1 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java @@ -67,9 +67,7 @@ public static SFClientConfig loadSFClientConfig(String configFilePath) throws IO } if (derivedConfigFilePath != null) { try { - if (!checkConfigFilePermissions(derivedConfigFilePath)) { - return null; - } + checkConfigFilePermissions(derivedConfigFilePath); File configFile = new File(derivedConfigFilePath); ObjectMapper objectMapper = new ObjectMapper(); @@ -124,28 +122,31 @@ && systemGetProperty("os.name").toLowerCase().startsWith("windows")) { } } - public static Boolean checkConfigFilePermissions(String derivedConfigFilePath) + + private static void checkConfigFilePermissions(String derivedConfigFilePath) throws IOException { try { if (Constants.getOS() != Constants.OS.WINDOWS) { // Check permissions of config file - Set folderPermissions = - Files.getPosixFilePermissions(Paths.get(derivedConfigFilePath)); - if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE) - || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE)) { + if (checkGroupOthersWritePermissions(derivedConfigFilePath)) { String error = String.format( "Error due to other users having permission to modify the config file: %s", derivedConfigFilePath); // TODO: SNOW-1503722 to change warning log to throw an error instead logger.warn(error); - return false; } } } catch (IOException e) { throw e; } - return true; + } + + static Boolean checkGroupOthersWritePermissions(String configFilePath) throws IOException { + Set folderPermissions = + Files.getPosixFilePermissions(Paths.get(configFilePath)); + return folderPermissions.contains(PosixFilePermission.GROUP_WRITE) + || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE); } static String convertToWindowsPath(String filePath) { diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java index a3d62647c..bdf7e138e 100644 --- a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -6,20 +6,22 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermissions; import java.util.HashMap; import java.util.Map; import java.util.Set; import net.snowflake.client.ConditionalIgnoreRule; import net.snowflake.client.RunningOnWin; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; +import net.snowflake.client.core.Constants; +import org.junit.*; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @RunWith(Parameterized.class) public class SFPermissionsTest { + @Rule + public ConditionalIgnoreRule rule = new ConditionalIgnoreRule(); @Parameterized.Parameters(name = "permission={0}") public static Set> data() { @@ -75,7 +77,7 @@ public void testLogDirectoryPermissions() throws IOException { // TODO: SNOW-1503722 Change to check for thrown exceptions // Don't run on Windows Files.setPosixFilePermissions(configFilePath, PosixFilePermissions.fromString(permission)); - Boolean result = SFClientConfigParser.checkConfigFilePermissions(configFilePath.toString()); + Boolean result = SFClientConfigParser.checkGroupOthersWritePermissions(configFilePath.toString()); if (isSucceed != result) { fail("testLogDirectoryPermissions failed. Expected " + isSucceed); } From 1906a2a6cfc849bd19672895aca0f4300d94e0b7 Mon Sep 17 00:00:00 2001 From: norrislee Date: Wed, 26 Jun 2024 23:27:59 -0700 Subject: [PATCH 17/19] Fix formatting --- .../net/snowflake/client/config/SFClientConfigParser.java | 8 +++----- .../net/snowflake/client/config/SFPermissionsTest.java | 8 +++----- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java index b96652ca1..a0ca0fa11 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java @@ -122,9 +122,7 @@ && systemGetProperty("os.name").toLowerCase().startsWith("windows")) { } } - - private static void checkConfigFilePermissions(String derivedConfigFilePath) - throws IOException { + private static void checkConfigFilePermissions(String derivedConfigFilePath) throws IOException { try { if (Constants.getOS() != Constants.OS.WINDOWS) { // Check permissions of config file @@ -144,9 +142,9 @@ private static void checkConfigFilePermissions(String derivedConfigFilePath) static Boolean checkGroupOthersWritePermissions(String configFilePath) throws IOException { Set folderPermissions = - Files.getPosixFilePermissions(Paths.get(configFilePath)); + Files.getPosixFilePermissions(Paths.get(configFilePath)); return folderPermissions.contains(PosixFilePermission.GROUP_WRITE) - || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE); + || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE); } static String convertToWindowsPath(String filePath) { diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java index bdf7e138e..9ba1194c3 100644 --- a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -6,22 +6,19 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; -import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermissions; import java.util.HashMap; import java.util.Map; import java.util.Set; import net.snowflake.client.ConditionalIgnoreRule; import net.snowflake.client.RunningOnWin; -import net.snowflake.client.core.Constants; import org.junit.*; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @RunWith(Parameterized.class) public class SFPermissionsTest { - @Rule - public ConditionalIgnoreRule rule = new ConditionalIgnoreRule(); + @Rule public ConditionalIgnoreRule rule = new ConditionalIgnoreRule(); @Parameterized.Parameters(name = "permission={0}") public static Set> data() { @@ -77,7 +74,8 @@ public void testLogDirectoryPermissions() throws IOException { // TODO: SNOW-1503722 Change to check for thrown exceptions // Don't run on Windows Files.setPosixFilePermissions(configFilePath, PosixFilePermissions.fromString(permission)); - Boolean result = SFClientConfigParser.checkGroupOthersWritePermissions(configFilePath.toString()); + Boolean result = + SFClientConfigParser.checkGroupOthersWritePermissions(configFilePath.toString()); if (isSucceed != result) { fail("testLogDirectoryPermissions failed. Expected " + isSucceed); } From 7f8481b650327f8ffa62d3ae725c8e0880ee065c Mon Sep 17 00:00:00 2001 From: norrislee Date: Wed, 26 Jun 2024 23:30:56 -0700 Subject: [PATCH 18/19] Remove * import --- .../java/net/snowflake/client/config/SFPermissionsTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java index 9ba1194c3..d9c2a7c6e 100644 --- a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -12,7 +12,10 @@ import java.util.Set; import net.snowflake.client.ConditionalIgnoreRule; import net.snowflake.client.RunningOnWin; -import org.junit.*; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; From aaf791f027e5c04847438d70e432fac20b1763fd Mon Sep 17 00:00:00 2001 From: norrislee Date: Wed, 26 Jun 2024 23:37:55 -0700 Subject: [PATCH 19/19] Fix Permissions test case --- .../client/config/SFPermissionsTest.java | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java index d9c2a7c6e..92ec8a624 100644 --- a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -28,24 +28,24 @@ public static Set> data() { Map testConfigFilePermissions = new HashMap() { { - put("rwx------", true); - put("rw-------", true); - put("r-x------", true); - put("r--------", true); - put("rwxrwx---", false); - put("rwxrw----", false); - put("rwxr-x---", true); - put("rwxr-----", true); - put("rwx-wx---", false); - put("rwx-w----", false); - put("rwx--x---", true); - put("rwx---rwx", false); - put("rwx---rw-", false); - put("rwx---r-x", true); - put("rwx---r--", true); - put("rwx----wx", false); - put("rwx----w-", false); - put("rwx-----x", true); + put("rwx------", false); + put("rw-------", false); + put("r-x------", false); + put("r--------", false); + put("rwxrwx---", true); + put("rwxrw----", true); + put("rwxr-x---", false); + put("rwxr-----", false); + put("rwx-wx---", true); + put("rwx-w----", true); + put("rwx--x---", false); + put("rwx---rwx", true); + put("rwx---rw-", true); + put("rwx---r-x", false); + put("rwx---r--", false); + put("rwx----wx", true); + put("rwx----w-", true); + put("rwx-----x", false); } }; return testConfigFilePermissions.entrySet();