# HG changeset patch # User Severin Gehwolf # Date 1377096351 -7200 # Node ID 01f01fb9f63db51081c93296311ab9681a131247 # Parent 264f429d20198d5440ba597e1206396a6cf1d679 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 diff -r 264f429d2019 -r 01f01fb9f63d integration-tests/src/test/java/com/redhat/thermostat/itest/WebAppTest.java --- 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); diff -r 264f429d2019 -r 01f01fb9f63d 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 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 { diff -r 264f429d2019 -r 01f01fb9f63d 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 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 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); }