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.