# HG changeset patch # User Severin Gehwolf # Date 1392312661 -3600 # Node ID ef11ae1f62705574c7249ba86dd81bcb39e0e692 # Parent d7118902cc688df574940935c34a19d5336e691e Do not create directory structure in WebStorageEndPoint. Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2014-February/009203.html Reviewed-by: vanaltj PR1637 diff -r d7118902cc68 -r ef11ae1f6270 web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java --- 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()); } diff -r d7118902cc68 -r ef11ae1f6270 web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java --- /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 + * . + * + * 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 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() { + + @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; + } + + }); + } +} diff -r d7118902cc68 -r ef11ae1f6270 web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java --- 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 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); }