changeset 1386:ef11ae1f6270

Do not create directory structure in WebStorageEndPoint. Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2014-February/009203.html Reviewed-by: vanaltj PR1637
author Severin Gehwolf <sgehwolf@redhat.com>
date Thu, 13 Feb 2014 18:31:01 +0100
parents d7118902cc68
children 366d8950cef3
files web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java
diffstat 3 files changed, 334 insertions(+), 93 deletions(-) [+]
line wrap: on
line diff
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java	Thu Feb 13 18:40:55 2014 +0100
+++ b/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java	Thu Feb 13 18:31:01 2014 +0100
@@ -71,7 +71,6 @@
 import com.google.gson.GsonBuilder;
 import com.redhat.thermostat.common.utils.LoggingUtils;
 import com.redhat.thermostat.shared.config.CommonPaths;
-import com.redhat.thermostat.shared.config.DirectoryStructureCreator;
 import com.redhat.thermostat.shared.config.InvalidConfigurationException;
 import com.redhat.thermostat.shared.config.internal.CommonPathsImpl;
 import com.redhat.thermostat.storage.core.Categories;
@@ -261,12 +260,12 @@
     }
 
     // Side effect: sets this.paths
-    private boolean isThermostatHomeSet() {
+    // package-private for testing
+    boolean isThermostatHomeSet() {
         try {
             // this throws config exception if neither the property
             // nor the env var is set
             paths = new CommonPathsImpl();
-            new DirectoryStructureCreator(paths).createPaths();
             return true;
         } catch (InvalidConfigurationException e) {
             return false;
@@ -281,15 +280,26 @@
             throw new RuntimeException(msg);
         }
         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()
-                    + " is not readable or does not exist!";
+                    + 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");
+        if (!sslProperties.canRead()) {
+            String msg = "File " + sslProperties.getAbsolutePath() +
+                    notReadableMsg;
+            logger.log(Level.SEVERE, msg);
+            throw new RuntimeException(msg);
+        }
+        // Thermost home looks OK and seems usable
         logger.log(Level.FINEST, "THERMOSTAT_HOME == "
                 + thermostatHomeFile.getAbsolutePath());
     }
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java	Thu Feb 13 18:31:01 2014 +0100
@@ -0,0 +1,268 @@
+/*
+ * Copyright 2012-2014 Red Hat, Inc.
+ *
+ * This file is part of Thermostat.
+ *
+ * Thermostat is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; either version 2, or (at your
+ * option) any later version.
+ *
+ * Thermostat is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Thermostat; see the file COPYING.  If not see
+ * <http://www.gnu.org/licenses/>.
+ *
+ * Linking this code with other modules is making a combined work
+ * based on this code.  Thus, the terms and conditions of the GNU
+ * General Public License cover the whole combination.
+ *
+ * As a special exception, the copyright holders of this code give
+ * you permission to link this code with independent modules to
+ * produce an executable, regardless of the license terms of these
+ * independent modules, and to copy and distribute the resulting
+ * executable under terms of your choice, provided that you also
+ * meet, for each linked independent module, the terms and conditions
+ * of the license of that module.  An independent module is a module
+ * which is not derived from or based on this code.  If you modify
+ * this code, you may extend this exception to your version of the
+ * library, but you are not obligated to do so.  If you do not wish
+ * to do so, delete this exception statement from your version.
+ */
+
+package com.redhat.thermostat.web.server;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.io.IOException;
+import java.lang.reflect.Method;
+import java.nio.file.FileVisitResult;
+import java.nio.file.FileVisitor;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.nio.file.attribute.FileAttribute;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.servlet.ServletConfig;
+import javax.servlet.ServletException;
+
+import org.junit.After;
+import org.junit.Test;
+
+import com.redhat.thermostat.web.server.auth.WebStoragePathHandler;
+
+/**
+ * A test class for {@link WebStorageEndPoint}. It should contain tests for
+ * which we don't need the servlet deployed in a container. I.e. which are
+ * more of the unit-test nature (rather than of the functional test nature).
+ *
+ */
+public class WebStorageEndPointUnitTest {
+
+    @After
+    public void tearDown() {
+        System.clearProperty("THERMOSTAT_HOME");
+    }
+    
+    /*
+     * 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("THERMOSTAT_HOME", "/root");
+        WebStorageEndPoint endpoint = new WebStorageEndPoint();
+        assertTrue("THERMOSTAT_HOME 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.
+     * 
+     * @throws Exception
+     */
+    @Test
+    public void ensureAuthorizationCovered() throws Exception {
+        // manually maintained list of path handlers which should include
+        // authorization checks
+        final String[] authPaths = new String[] {
+                "prepare-statement", "query-execute", "write-execute", "register-category",
+                "save-file", "load-file", "purge", "ping", "generate-token", "verify-token"
+        };
+        Map<String, Boolean> checkedAutPaths = new HashMap<>();
+        for (String path: authPaths) {
+            checkedAutPaths.put(path, false);
+        }
+        int methodsReqAuthorization = 0;
+        for (Method method: WebStorageEndPoint.class.getDeclaredMethods()) {
+            if (method.isAnnotationPresent(WebStoragePathHandler.class)) {
+                methodsReqAuthorization++;
+                WebStoragePathHandler annot = method.getAnnotation(WebStoragePathHandler.class);
+                try {
+                    // this may NPE if there is something funny going on in
+                    // WebStorageEndPoint (e.g. one method annotated but this
+                    // reference list has not been updated).
+                    if (!checkedAutPaths.get(annot.path())) {
+                        // mark path as covered
+                        checkedAutPaths.put(annot.path(), true);
+                    } else {
+                        throw new AssertionError(
+                                "method "
+                                        + method
+                                        + " annotated as web storage path handler (path '"
+                                        + annot.path()
+                                        + "'), but not in reference list we know about!");
+                    }
+                } catch (NullPointerException e) {
+                    throw new AssertionError("Don't know about path '"
+                            + annot.path() + "'");
+                }
+            }
+        }
+        // at this point we should have all dispatched paths covered
+        for (String path: authPaths) {
+            assertTrue(
+                    "Is " + path
+                          + " marked with @WebStoragePathHandler and have proper authorization checks been included?",
+                    checkedAutPaths.get(path));
+        }
+        assertEquals(authPaths.length, methodsReqAuthorization);
+    }
+    
+    @Test
+    public void initThrowsRuntimeExceptionIfThermostatHomeNotSet() {
+        WebStorageEndPoint endpoint = new WebStorageEndPoint();
+        ServletConfig config = mock(ServletConfig.class);
+        try {
+            endpoint.init(config);
+            fail("Thermostat home was not set in config, should not get here!");
+        } catch (RuntimeException e) {
+            // pass
+            assertTrue(e.getMessage().contains("THERMOSTAT_HOME"));
+        } catch (ServletException e) {
+            fail(e.getMessage());
+        }
+        // set config with non-existing dir
+        when(config.getInitParameter("THERMOSTAT_HOME")).thenReturn("not-existing");
+        try {
+            endpoint.init(config);
+            fail("Thermostat home was set in config but file does not exist, should have died!");
+        } catch (RuntimeException e) {
+            // pass
+            assertTrue(e.getMessage().contains("THERMOSTAT_HOME"));
+        } catch (ServletException e) {
+            fail(e.getMessage());
+        }
+    }
+    
+    @Test
+    public void initThrowsRuntimeExceptionIfSSLPropertiesNotReadable() throws Exception {
+        Path testThermostatHome = null;
+        File etcDir = null;
+        try {
+            testThermostatHome = Files.createTempDirectory(
+                    "foo-thermostat-home-", new FileAttribute[] {});
+            File thFile = testThermostatHome.toFile();
+            etcDir = new File(thFile, "etc");
+            etcDir.mkdir();
+            assertTrue(etcDir.exists());
+            assertTrue(etcDir.canWrite());
+            File sslProperties = new File(etcDir, "ssl.properties");
+            sslProperties.createNewFile();
+            assertTrue(sslProperties.canRead());
+            // explicitly remove read perms from etc directory
+            etcDir.setExecutable(false);
+            assertFalse(sslProperties.canRead());
+
+            WebStorageEndPoint endpoint = new WebStorageEndPoint();
+            System.setProperty("THERMOSTAT_HOME", thFile.getAbsolutePath());
+            try {
+                endpoint.init(mock(ServletConfig.class));
+                fail("should have failed to initialize! can't read ssl.properties");
+            } catch (RuntimeException e) {
+                assertTrue(e.getMessage().contains("ssl.properties"));
+            }
+        } finally {
+            etcDir.setExecutable(true);
+            if (testThermostatHome != null) {
+                deleteDirectoryRecursive(testThermostatHome);
+            }
+        }
+    }
+    
+    @Test
+    public void initThrowsRuntimeExceptionIfSSLPropertiesDoesnotExist() throws Exception {
+        Path testThermostatHome = null;
+        try {
+            testThermostatHome = Files.createTempDirectory(
+                    "bar-thermostat-home-", new FileAttribute[] {});
+            File thFile = testThermostatHome.toFile();
+            WebStorageEndPoint endpoint = new WebStorageEndPoint();
+            System.setProperty("THERMOSTAT_HOME", thFile.getAbsolutePath());
+            try {
+                endpoint.init(mock(ServletConfig.class));
+                fail("should have failed to initialize, ssl.properties not existing!");
+            } catch (RuntimeException e) {
+                assertTrue(e.getMessage().contains("ssl.properties"));
+            }
+        } finally {
+            if (testThermostatHome != null) {
+                deleteDirectoryRecursive(testThermostatHome);
+            }
+        }
+    }
+    
+    private void deleteDirectoryRecursive(Path dir) throws IOException {
+        Files.walkFileTree(dir, new FileVisitor<Path>() {
+
+            @Override
+            public FileVisitResult preVisitDirectory(Path dir,
+                    BasicFileAttributes attrs) throws IOException {
+                // nothing
+                return FileVisitResult.CONTINUE;
+            }
+
+            @Override
+            public FileVisitResult visitFile(Path file,
+                    BasicFileAttributes attrs) throws IOException {
+                Files.delete(file);
+                return FileVisitResult.CONTINUE;
+            }
+
+            @Override
+            public FileVisitResult visitFileFailed(Path file, IOException exc)
+                    throws IOException {
+                exc.printStackTrace();
+                return FileVisitResult.CONTINUE;
+            }
+
+            @Override
+            public FileVisitResult postVisitDirectory(Path dir, IOException exc)
+                    throws IOException {
+                // All files have been visitated before that.
+                Files.delete(dir);
+                return FileVisitResult.CONTINUE;
+            }
+            
+        });
+    }
+}
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java	Thu Feb 13 18:40:55 2014 +0100
+++ b/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java	Thu Feb 13 18:31:01 2014 +0100
@@ -48,13 +48,13 @@
 import static org.mockito.Mockito.when;
 
 import java.io.ByteArrayInputStream;
+import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.io.OutputStreamWriter;
 import java.io.Reader;
-import java.lang.reflect.Method;
 import java.lang.reflect.Type;
 import java.net.HttpURLConnection;
 import java.net.MalformedURLException;
@@ -63,15 +63,11 @@
 import java.net.URLEncoder;
 import java.security.Principal;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 
 import javax.security.auth.Subject;
-import javax.servlet.ServletConfig;
-import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletResponse;
 
 import org.apache.commons.codec.binary.Base64;
@@ -81,6 +77,8 @@
 import org.eclipse.jetty.security.MappedLoginService;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.UserIdentity;
+import org.eclipse.jetty.util.component.LifeCycle;
+import org.eclipse.jetty.util.component.LifeCycle.Listener;
 import org.eclipse.jetty.util.security.Password;
 import org.eclipse.jetty.webapp.WebAppContext;
 import org.junit.After;
@@ -134,7 +132,6 @@
 import com.redhat.thermostat.web.server.auth.RolePrincipal;
 import com.redhat.thermostat.web.server.auth.Roles;
 import com.redhat.thermostat.web.server.auth.UserPrincipal;
-import com.redhat.thermostat.web.server.auth.WebStoragePathHandler;
 
 public class WebStorageEndpointTest {
 
@@ -206,8 +203,56 @@
 
     private void startServer(int port, LoginService loginService) throws Exception {
         server = new Server(port);
-        WebAppContext ctx = new WebAppContext("src/main/webapp", "/");
+        // This makes the test use the web.xml in src/main/webapp. That's
+        // also where the test's thermostat home is set up via 
+        // the context listener. THERMOSTAT_HOME == /tmp/test-thermostat-home
+        // See src/main/webapp/WEB-INF/web.xml
+        final WebAppContext ctx = new WebAppContext("src/main/webapp", "/");
         ctx.getSecurityHandler().setLoginService(loginService);
+        // We get access to the THERMOSTAT_HOME value once the context has
+        // been started.
+        ctx.addLifeCycleListener(new Listener() {
+
+            @Override
+            public void lifeCycleFailure(LifeCycle arg0, Throwable arg1) {
+                // nothing
+            }
+
+            @Override
+            public void lifeCycleStarted(LifeCycle arg0) {
+                // In Servlet.init() we check if ssl.properties exists. We need
+                // to create that file in order for the tests to get past this
+                // check.
+                File thermostatHome = new File(ctx.getInitParameter("THERMOSTAT_HOME"));
+                File configDirectory = new File(thermostatHome, "etc");
+                if (!configDirectory.exists()) {
+                    configDirectory.mkdir();
+                }
+                File sslProperties = new File(configDirectory, "ssl.properties");
+                // only creates file if it doesn't exist yet
+                try {
+                    sslProperties.createNewFile();
+                } catch (IOException e) {
+                    throw new RuntimeException(e);
+                }
+            }
+
+            @Override
+            public void lifeCycleStarting(LifeCycle arg0) {
+                // nothing
+            }
+
+            @Override
+            public void lifeCycleStopped(LifeCycle arg0) {
+                // nothing
+            }
+
+            @Override
+            public void lifeCycleStopping(LifeCycle arg0) {
+                // nothing
+            }
+            
+        });
         server.setHandler(ctx);
         server.start();
     }
@@ -224,60 +269,6 @@
         KnownDescriptorRegistryFactory.setKnownDescriptorRegistry(null);
     }
 
-    /**
-     * Makes sure that all paths we dispatch to, dispatch to
-     * {@link WebStoragePathHandler} annotated methods.
-     * 
-     * @throws Exception
-     */
-    @Test
-    public void ensureAuthorizationCovered() throws Exception {
-        // manually maintained list of path handlers which should include
-        // authorization checks
-        final String[] authPaths = new String[] {
-                "prepare-statement", "query-execute", "write-execute", "register-category",
-                "save-file", "load-file", "purge", "ping", "generate-token", "verify-token"
-        };
-        Map<String, Boolean> checkedAutPaths = new HashMap<>();
-        for (String path: authPaths) {
-            checkedAutPaths.put(path, false);
-        }
-        int methodsReqAuthorization = 0;
-        for (Method method: WebStorageEndPoint.class.getDeclaredMethods()) {
-            if (method.isAnnotationPresent(WebStoragePathHandler.class)) {
-                methodsReqAuthorization++;
-                WebStoragePathHandler annot = method.getAnnotation(WebStoragePathHandler.class);
-                try {
-                    // this may NPE if there is something funny going on in
-                    // WebStorageEndPoint (e.g. one method annotated but this
-                    // reference list has not been updated).
-                    if (!checkedAutPaths.get(annot.path())) {
-                        // mark path as covered
-                        checkedAutPaths.put(annot.path(), true);
-                    } else {
-                        throw new AssertionError(
-                                "method "
-                                        + method
-                                        + " annotated as web storage path handler (path '"
-                                        + annot.path()
-                                        + "'), but not in reference list we know about!");
-                    }
-                } catch (NullPointerException e) {
-                    throw new AssertionError("Don't know about path '"
-                            + annot.path() + "'");
-                }
-            }
-        }
-        // at this point we should have all dispatched paths covered
-        for (String path: authPaths) {
-            assertTrue(
-                    "Is " + path
-                          + " marked with @WebStoragePathHandler and have proper authorization checks been included?",
-                    checkedAutPaths.get(path));
-        }
-        assertEquals(authPaths.length, methodsReqAuthorization);
-    }
-    
     @Test
     public void authorizedPrepareQueryWithUnTrustedDescriptor() throws Exception {
         String strDescriptor = "QUERY " + category.getName() + " WHERE '" + key1.getName() + "' = ?s SORT '" + key1.getName() + "' DSC LIMIT 42";
@@ -1492,34 +1483,6 @@
         doUnauthorizedTest("verify-token", failMsg, insufficientRoles, false);
     }
     
-    @Test
-    public void initThrowsRuntimeExceptionIfThermostatHomeNotSet() {
-        // setup sets this, but we don't want to have it set for this test
-        System.clearProperty("THERMOSTAT_HOME");
-        WebStorageEndPoint endpoint = new WebStorageEndPoint();
-        ServletConfig config = mock(ServletConfig.class);
-        try {
-            endpoint.init(config);
-            fail("Thermostat home was not set in config, should not get here!");
-        } catch (RuntimeException e) {
-            // pass
-            assertTrue(e.getMessage().contains("THERMOSTAT_HOME"));
-        } catch (ServletException e) {
-            fail(e.getMessage());
-        }
-        // set config with non-existing dir
-        when(config.getInitParameter("THERMOSTAT_HOME")).thenReturn("not-existing");
-        try {
-            endpoint.init(config);
-            fail("Thermostat home was set in config but file does not exist, should have died!");
-        } catch (RuntimeException e) {
-            // pass
-            assertTrue(e.getMessage().contains("THERMOSTAT_HOME"));
-        } catch (ServletException e) {
-            fail(e.getMessage());
-        }
-    }
-
     private byte[] verifyAuthorizedGenerateToken(String username, String password, String actionName) throws IOException {
         return verifyAuthorizedGenerateToken(username, password, actionName, 200);
     }