Mercurial > hg > release > thermostat-0.15

changeset 1258:01f01fb9f63d

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
PR1527: Improve ACL checks for Storage.saveFile() and Storage.loadFile(). Reviewed-by: omajid Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2013-August/007998.html
author Severin Gehwolf <sgehwolf@redhat.com>
date Wed, 21 Aug 2013 16:45:51 +0200
parents 264f429d2019
children 7d6cd64ffd0c
files integration-tests/src/test/java/com/redhat/thermostat/itest/WebAppTest.java web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java
diffstat 3 files changed, 179 insertions(+), 13 deletions(-) [+]
line wrap: on
line diff
--- a/integration-tests/src/test/java/com/redhat/thermostat/itest/WebAppTest.java	Fri Sep 20 12:21:31 2013 -0400
+++ b/integration-tests/src/test/java/com/redhat/thermostat/itest/WebAppTest.java	Wed Aug 21 16:45:51 2013 +0200
@@ -972,7 +972,9 @@
                 Roles.LOAD_FILE,
                 Roles.SAVE_FILE,
                 Roles.ACCESS_REALM,
-                Roles.LOGIN
+                Roles.LOGIN,
+                Roles.GRANT_FILES_WRITE_ALL,
+                Roles.GRANT_FILES_READ_ALL
         };
         Storage webStorage = getAndConnectStorage(TEST_USER, TEST_PASSWORD, roleNames);
         
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java	Fri Sep 20 12:21:31 2013 -0400
+++ b/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java	Wed Aug 21 16:45:51 2013 +0200
@@ -124,6 +124,8 @@
 public class WebStorageEndPoint extends HttpServlet {
 
     static final String CMDC_AUTHORIZATION_GRANT_ROLE_PREFIX = "thermostat-cmdc-grant-";
+    static final String FILES_READ_GRANT_ROLE_PREFIX = "thermostat-files-grant-read-filename-";
+    static final String FILES_WRITE_GRANT_ROLE_PREFIX = "thermostat-files-grant-write-filename-";
     private static final String TOKEN_MANAGER_TIMEOUT_PARAM = "token-manager-timeout";
     private static final String TOKEN_MANAGER_KEY = "token-manager";
     private static final String JETTY_JAAS_USER_PRINCIPAL_CLASS_NAME = "org.eclipse.jetty.plus.jaas.JAASUserPrincipal";
@@ -401,6 +403,9 @@
         }
         
         String name = req.getParameter("file");
+        if (! isAllowedToLoadFile(req, resp, name)) {
+            return;
+        }
         try (InputStream data = storage.loadFile(name)) {
             if (data == null) {
                 resp.setStatus(HttpServletResponse.SC_NO_CONTENT);
@@ -438,6 +443,9 @@
                 String fieldName = item.getFieldName();
                 if (fieldName.equals("file")) {
                     String name = item.getName();
+                    if (! isAllowedToSaveFile(req, resp, name)) {
+                        return;
+                    }
                     InputStream in = item.getInputStream();
                     storage.saveFile(name, in);
                 }
@@ -448,6 +456,35 @@
         
     }
 
+    private boolean isAllowedToLoadFile(HttpServletRequest req,
+            HttpServletResponse resp, String filename) {
+        String fileRole = FILES_READ_GRANT_ROLE_PREFIX + filename;
+        return isAllowed(req, resp, filename, Roles.GRANT_FILES_READ_ALL, fileRole);
+        
+    }
+
+    private boolean isAllowedToSaveFile(HttpServletRequest req,
+            HttpServletResponse resp, String filename) {
+        String fileRole = FILES_WRITE_GRANT_ROLE_PREFIX + filename;
+        return isAllowed(req, resp, filename, Roles.GRANT_FILES_WRITE_ALL, fileRole);
+    }
+
+    private boolean isAllowed(HttpServletRequest req, HttpServletResponse resp,
+            String filename, String grantAllRole, String specificFileRole) {
+        if (req.isUserInRole(grantAllRole) || req.isUserInRole(specificFileRole)) {
+            return true;
+        } else {
+            String detailMsg = "User '" + req.getRemoteUser() +
+                    "' does not belong to any of the following roles: [ " + 
+                    grantAllRole + ", " + 
+                    specificFileRole + " ]";
+            logger.log(Level.INFO, "Permission denied for file '" +
+                    filename + "'. " + detailMsg);
+            resp.setStatus(HttpServletResponse.SC_FORBIDDEN);
+            return false;
+        }
+    }
+
     @SuppressWarnings("unchecked") // need to adapt categories
     @WebStoragePathHandler( path = "register-category" )
     private synchronized void registerCategory(HttpServletRequest req, HttpServletResponse resp) throws IOException {
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java	Fri Sep 20 12:21:31 2013 -0400
+++ b/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java	Wed Aug 21 16:45:51 2013 +0200
@@ -1227,9 +1227,12 @@
 
     @Test
     public void authorizedSaveFile() throws Exception {
+        String filename = "fluff";
         String[] roleNames = new String[] {
                 Roles.SAVE_FILE,
-                Roles.ACCESS_REALM
+                Roles.ACCESS_REALM,
+                // User also needs permission for specific file to be saved.
+                WebStorageEndPoint.FILES_WRITE_GRANT_ROLE_PREFIX + filename
         };
         String testuser = "testuser";
         String password = "testpassword";
@@ -1252,7 +1255,7 @@
         conn.setRequestProperty("Transfer-Encoding", "chunked");
         OutputStreamWriter out = new OutputStreamWriter(conn.getOutputStream());
         out.write("--fluff\r\n");
-        out.write("Content-Disposition: form-data; name=\"file\"; filename=\"fluff\"\r\n");
+        out.write("Content-Disposition: form-data; name=\"file\"; filename=\"" + filename + "\"\r\n");
         out.write("Content-Type: application/octet-stream\r\n");
         out.write("Content-Transfer-Encoding: binary\r\n");
         out.write("\r\n");
@@ -1260,9 +1263,10 @@
         out.write("--fluff--\r\n");
         out.flush();
         // needed in order to trigger inCaptor interaction with mock
-        conn.getResponseCode();
+        int respCode = conn.getResponseCode();
+        assertEquals(HttpServletResponse.SC_OK, respCode);
         ArgumentCaptor<InputStream> inCaptor = ArgumentCaptor.forClass(InputStream.class);
-        verify(mockStorage).saveFile(eq("fluff"), inCaptor.capture());
+        verify(mockStorage).saveFile(eq(filename), inCaptor.capture());
         InputStream in = inCaptor.getValue();
         byte[] data = new byte[11];
         int totalRead = 0;
@@ -1279,15 +1283,60 @@
     @Test
     public void unauthorizedSaveFile() throws Exception {
         String failMsg = "thermostat-save-file role missing, expected Forbidden!";
-        String[] insufficientRoles = new String[0];
+        String[] insufficientRoles = new String[] {
+                Roles.ACCESS_REALM
+        };
         doUnauthorizedTest("save-file", failMsg, insufficientRoles, false);
     }
+    
+    @Test
+    public void unauthorizedSaveFileMissingSpecificRole() throws Exception {
+        String filename = "foo.txt";
+        String[] insufficientRoles = new String[] {
+                Roles.SAVE_FILE,
+                Roles.ACCESS_REALM
+        };
+        String testuser = "testuser";
+        String password = "testpassword";
+        final LoginService loginService = new TestLoginService(testuser, password, insufficientRoles); 
+        port = FreePortFinder.findFreePort(new TryPort() {
+            
+            @Override
+            public void tryPort(int port) throws Exception {
+                startServer(port, loginService);
+            }
+        });
+        String endpoint = getEndpoint();
+
+        URL url = new URL(endpoint + "/save-file");
+        HttpURLConnection conn = (HttpURLConnection) url.openConnection();
+        conn.setRequestMethod("POST");
+        sendAuthentication(conn, testuser, password);
+        conn.setDoOutput(true);
+        conn.setRequestProperty("Content-Type", "multipart/form-data; boundary=fluff");
+        conn.setRequestProperty("Transfer-Encoding", "chunked");
+        OutputStreamWriter out = new OutputStreamWriter(conn.getOutputStream());
+        out.write("--fluff\r\n");
+        out.write("Content-Disposition: form-data; name=\"file\"; filename=\"" + filename + "\"\r\n");
+        out.write("Content-Type: application/octet-stream\r\n");
+        out.write("Content-Transfer-Encoding: binary\r\n");
+        out.write("\r\n");
+        out.write("Hello World\r\n");
+        out.write("--fluff--\r\n");
+        out.flush();
+        int respCode = conn.getResponseCode();
+        assertEquals(HttpServletResponse.SC_FORBIDDEN, respCode);
+        verifyNoMoreInteractions(mockStorage);
+    }
 
     @Test
     public void authorizedLoadFile() throws Exception {
+        String filename = "fluff";
         String[] roleNames = new String[] {
                 Roles.LOAD_FILE,
-                Roles.ACCESS_REALM
+                Roles.ACCESS_REALM,
+                // Grant the specific read file permission
+                WebStorageEndPoint.FILES_READ_GRANT_ROLE_PREFIX + filename
         };
         String testuser = "testuser";
         String password = "testpassword";
@@ -1302,7 +1351,7 @@
         
         byte[] data = "Hello World".getBytes();
         InputStream in = new ByteArrayInputStream(data);
-        when(mockStorage.loadFile("fluff")).thenReturn(in);
+        when(mockStorage.loadFile(filename)).thenReturn(in);
 
         String endpoint = getEndpoint();
         URL url = new URL(endpoint + "/load-file");
@@ -1313,8 +1362,10 @@
         conn.setDoOutput(true);
         conn.setDoInput(true);
         OutputStreamWriter out = new OutputStreamWriter(conn.getOutputStream());
-        out.write("file=fluff");
+        out.write("file=" + filename);
         out.flush();
+        int respCode = conn.getResponseCode();
+        assertEquals(HttpServletResponse.SC_OK, respCode);
         in = conn.getInputStream();
         data = new byte[11];
         int totalRead = 0;
@@ -1326,15 +1377,55 @@
             totalRead += read;
         }
         assertEquals("Hello World", new String(data));
-        verify(mockStorage).loadFile("fluff");
+        verify(mockStorage).loadFile(filename);
     }
     
     @Test
     public void unauthorizedLoadFile() throws Exception {
         String failMsg = "thermostat-load-file role missing, expected Forbidden!";
-        String[] insufficientRoles = new String[0];
+        String[] insufficientRoles = new String[] {
+                Roles.ACCESS_REALM
+        };
         doUnauthorizedTest("load-file", failMsg, insufficientRoles, false);
     }
+    
+    @Test
+    public void unauthorizedLoadFileMissingSpecificRole() throws Exception {
+        String filename = "foo.txt";
+        String[] insufficientRoles = new String[] {
+                Roles.LOAD_FILE,
+                Roles.ACCESS_REALM
+        };
+        String testuser = "testuser";
+        String password = "testpassword";
+        final LoginService loginService = new TestLoginService(testuser, password, insufficientRoles); 
+        port = FreePortFinder.findFreePort(new TryPort() {
+            
+            @Override
+            public void tryPort(int port) throws Exception {
+                startServer(port, loginService);
+            }
+        });
+        
+        byte[] data = "Hello World".getBytes();
+        InputStream in = new ByteArrayInputStream(data);
+        when(mockStorage.loadFile(filename)).thenReturn(in);
+
+        String endpoint = getEndpoint();
+        URL url = new URL(endpoint + "/load-file");
+
+        HttpURLConnection conn = (HttpURLConnection) url.openConnection();
+        conn.setRequestMethod("POST");
+        sendAuthentication(conn, testuser, password);
+        conn.setDoOutput(true);
+        conn.setDoInput(true);
+        OutputStreamWriter out = new OutputStreamWriter(conn.getOutputStream());
+        out.write("file=" + filename);
+        out.flush();
+        int respCode = conn.getResponseCode();
+        assertEquals(HttpServletResponse.SC_FORBIDDEN, respCode);
+        verifyNoMoreInteractions(mockStorage);
+    }
 
     @Test
     public void authorizedPurge() throws Exception {
@@ -1360,14 +1451,50 @@
         sendAuthentication(conn, testuser, password);
         conn.getOutputStream().write("agentId=fluff".getBytes());
         int status = conn.getResponseCode();
-        assertEquals(200, status);
+        assertEquals(HttpServletResponse.SC_OK, status);
         verify(mockStorage).purge("fluff");
     }
     
     @Test
+    public void unauthorizedAccessRealm() throws Exception {
+        String failMsg = Roles.ACCESS_REALM + " role missing, expected Forbidden!";
+        String[] insufficientRoles = new String[0];
+        // entry point for this test doesn't matter. Use '/'. 
+        doUnauthorizedTest("", failMsg, insufficientRoles, false);
+    }
+    
+    @Test
+    public void authorizedAccessRealm() throws Exception {
+        String[] roles = new String[] {
+                Roles.ACCESS_REALM
+        };
+        String testuser = "testuser";
+        String password = "testpassword";
+        final LoginService loginService = new TestLoginService(testuser, password, roles); 
+        port = FreePortFinder.findFreePort(new TryPort() {
+            
+            @Override
+            public void tryPort(int port) throws Exception {
+                startServer(port, loginService);
+            }
+        });
+        
+        String endpoint = getEndpoint();
+        URL url = new URL(endpoint + "/"); // Testing the realm, nothing else.
+        HttpURLConnection conn = (HttpURLConnection) url.openConnection();
+        conn.setRequestMethod("POST");
+        sendAuthentication(conn, testuser, password);
+        conn.setRequestProperty("Content-Type", "application/x-www-form-urlencoded");
+        
+        assertEquals(HttpServletResponse.SC_OK, conn.getResponseCode());
+    }
+    
+    @Test
     public void unauthorizedPurge() throws Exception {
         String failMsg = "thermostat-purge role missing, expected Forbidden!";
-        String[] insufficientRoles = new String[0];
+        String[] insufficientRoles = new String[] {
+                Roles.ACCESS_REALM
+        };
         doUnauthorizedTest("purge", failMsg, insufficientRoles, false);
     }