Mercurial > hg > thermostat
changeset 1605:295f5692a696
Don't duplicate THERMOSTAT_HOME sanity checks
reviewed-by: jerboaa
review-thread: http://icedtea.classpath.org/pipermail/thermostat/2014-December/012077.html
Remove duplicated sanity check in WebStorageEndPoint, move
readability check from EndPoint to more appropriate
CommonPathsImpl.
author | Jon VanAlten <jon.vanalten@redhat.com> |
---|---|
date | Fri, 05 Dec 2014 21:52:05 -0700 |
parents | 673e0738ffbd |
children | c053ee51a8c0 |
files | config/src/main/java/com/redhat/thermostat/shared/config/internal/CommonPathsImpl.java config/src/main/java/com/redhat/thermostat/shared/locale/internal/LocaleResources.java config/src/main/resources/com/redhat/thermostat/shared/locale/internal/strings.properties config/src/test/java/com/redhat/thermostat/shared/config/internal/CommonPathsImplTest.java web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java |
diffstat | 6 files changed, 37 insertions(+), 46 deletions(-) [+] |
line wrap: on
line diff
--- a/config/src/main/java/com/redhat/thermostat/shared/config/internal/CommonPathsImpl.java Fri Dec 05 20:33:58 2014 -0700 +++ b/config/src/main/java/com/redhat/thermostat/shared/config/internal/CommonPathsImpl.java Fri Dec 05 21:52:05 2014 -0700 @@ -121,6 +121,9 @@ if (!systemHome.exists()) { throw new InvalidConfigurationException(t.localize(LocaleResources.SYSHOME_DOESNT_EXIST, home)); } + if (!systemHome.canRead()) { + throw new InvalidConfigurationException(t.localize(LocaleResources.SYSHOME_CANNOT_READ, home)); + } if (!systemHome.isDirectory()) { throw new InvalidConfigurationException(t.localize(LocaleResources.SYSHOME_NOT_A_DIR, home)); }
--- a/config/src/main/java/com/redhat/thermostat/shared/locale/internal/LocaleResources.java Fri Dec 05 20:33:58 2014 -0700 +++ b/config/src/main/java/com/redhat/thermostat/shared/locale/internal/LocaleResources.java Fri Dec 05 21:52:05 2014 -0700 @@ -42,6 +42,7 @@ SYSHOME_NO_HOME, SYSHOME_DOESNT_EXIST, + SYSHOME_CANNOT_READ, SYSHOME_NOT_A_DIR, USERHOME_NOT_A_DIR, GENERAL_NOT_A_DIR,
--- a/config/src/main/resources/com/redhat/thermostat/shared/locale/internal/strings.properties Fri Dec 05 20:33:58 2014 -0700 +++ b/config/src/main/resources/com/redhat/thermostat/shared/locale/internal/strings.properties Fri Dec 05 21:52:05 2014 -0700 @@ -1,5 +1,6 @@ SYSHOME_NO_HOME = THERMOSTAT_HOME not defined... SYSHOME_DOESNT_EXIST = THERMOSTAT_HOME is defined but does not exist: {0} +SYSHOME_CANNOT_READ = THERMOSTAT_HOME exists but cannot be read: {0} SYSHOME_NOT_A_DIR = THERMOSTAT_HOME is defined but not a directory: {0} USERHOME_NOT_A_DIR = USER_THERMOSTAT_HOME is defined but not a directory: {0} GENERAL_NOT_A_DIR = Expecting directory, got other: {0}
--- a/config/src/test/java/com/redhat/thermostat/shared/config/internal/CommonPathsImplTest.java Fri Dec 05 20:33:58 2014 -0700 +++ b/config/src/test/java/com/redhat/thermostat/shared/config/internal/CommonPathsImplTest.java Fri Dec 05 21:52:05 2014 -0700 @@ -37,12 +37,13 @@ package com.redhat.thermostat.shared.config.internal; import static org.junit.Assert.fail; - import static com.redhat.thermostat.testutils.TestUtils.deleteRecursively; import java.io.File; import java.io.IOException; import java.nio.file.Files; +import java.nio.file.attribute.FileAttribute; +import java.nio.file.attribute.PosixFilePermissions; import org.junit.After; import org.junit.Assert; @@ -83,8 +84,8 @@ } } - private String setupTempDir(String tempPrefix, String propertyKey) throws IOException { - File tmpDir = Files.createTempDirectory(tempPrefix).toFile(); + private String setupTempDir(String tempPrefix, String propertyKey, FileAttribute<?>... attrs) throws IOException { + File tmpDir = Files.createTempDirectory(tempPrefix, attrs).toFile(); if (!tmpDir.exists()) { tmpDir.mkdirs(); } @@ -268,5 +269,27 @@ // pass } } + + @Test + public void instantiationThrowsExceptionThermostatHomeNotReadable() throws IOException { + String thermostatHome = null; + try { + thermostatHome = setupTempDir("CommonPathsImplTest.instantiationThrowsExceptionThermostatHomeNotReadable", + THERMOSTAT_HOME_PROPERTY, + PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("-wx--x---"))); + new CommonPathsImpl(); + fail("Should have thrown InvalidConfigurationException"); + } catch (InvalidConfigurationException e) { + // pass + } finally { + if (thermostatHome != null) { + // Don't use deleteTempDir because it can't list contents of unreadable dir + File tmpDir = new File(thermostatHome); + if (tmpDir.exists()) { + tmpDir.delete(); + } + } + } + } }
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java Fri Dec 05 20:33:58 2014 -0700 +++ b/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java Fri Dec 05 21:52:05 2014 -0700 @@ -166,7 +166,7 @@ // check if thermostat home is set and readable // Side effect: sets this.paths - checkThermostatHome(); + sanityCheckNecessaryFiles(); gson = new GsonBuilder() .registerTypeAdapterFactory(new PojoTypeAdapterFactory()) @@ -277,36 +277,17 @@ } // Side effect: sets this.paths - // package-private for testing - boolean isThermostatHomeSet() { + private void sanityCheckNecessaryFiles() { try { - // this throws config exception if neither the property - // nor the env var is set + // Throws config exception if basic sanity checks for + // THERMOSTAT_HOME don't pass. paths = new CommonPathsImpl(); - return true; } catch (InvalidConfigurationException e) { - return false; - } - } - - // Side effect: sets this.paths - private void checkThermostatHome() { - if (!isThermostatHomeSet()) { - String msg = "THERMOSTAT_HOME context parameter not set!"; - logger.log(Level.SEVERE, msg); - throw new RuntimeException(msg); + logger.log(Level.SEVERE, e.getMessage()); + throw new RuntimeException(e); } File thermostatHomeFile = getThermostatHome(); String notReadableMsg = " is not readable or does not exist!"; - if (!thermostatHomeFile.canRead()) { - // This is bad news. If we can't at least read THERMOSTAT_HOME - // we are bound to fail in some weird ways at some later point. - String msg = "THERMOSTAT_HOME = " - + thermostatHomeFile.getAbsolutePath() - + notReadableMsg; - logger.log(Level.SEVERE, msg); - throw new RuntimeException(msg); - } // we need to be able to read ssl config for backing storage // paths got set in isThermostatHomeSet() File sslProperties = new File(paths.getSystemConfigurationDirectory(), "ssl.properties");
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java Fri Dec 05 20:33:58 2014 -0700 +++ b/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java Fri Dec 05 21:52:05 2014 -0700 @@ -79,24 +79,6 @@ System.clearProperty(TH_HOME_PROP_NAME); } - /* - * Tests whether isThermostatHomeSet() works as expected. - * - * In particular, this tests sets thermostat home to a known-to-be-read-only - * for non-root users. That will make isThermostatHomeSet() return false - * due to creating paths failing. - * - * Regression test for the following bug: - * http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1637 - */ - @Test - public void testCheckThermostatHome() { - System.setProperty(TH_HOME_PROP_NAME, "/root"); - WebStorageEndPoint endpoint = new WebStorageEndPoint(); - assertTrue(TH_HOME_PROP_NAME + " clearly set, do we create paths where we shouldn't?", - endpoint.isThermostatHomeSet()); - } - /** * Makes sure that all paths we dispatch to, dispatch to * {@link WebStoragePathHandler} annotated methods.