diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java index 3c09a612f42..0df06056e8c 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java @@ -566,7 +566,6 @@ public Response cloneNote(@PathParam("noteId") String noteId, String message) throws IOException, IllegalArgumentException { LOGGER.info("Clone note by JSON {}", message); - checkIfUserCanWrite(noteId, "Insufficient privileges you cannot clone this note"); NewNoteRequest request = GSON.fromJson(message, NewNoteRequest.class); String newNoteName = null; String revisionId = null; diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java b/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java index 9e5e31aa1ce..da6203c7566 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java @@ -339,6 +339,16 @@ public String cloneNote(String noteId, String newNotePath, ServiceContext context, ServiceCallback callback) throws IOException { + // NotebookRestApi used to check write access here with hasWritePermission, which also grants + // access when Zeppelin runs without conf/shiro.ini. checkPermission does not, so it would + // reject a WebSocket clone that the REST endpoint still allows on an anonymous deployment. + if (!authorizationService.hasWritePermission(context.getUserAndRoles(), noteId)) { + callback.onFailure(new ForbiddenException("Insufficient privileges to clone note " + noteId + + ".\nAllowed users or roles: " + authorizationService.getWriters(noteId) + + "\nBut the user " + context.getAutheInfo().getUser() + " belongs to: " + + context.getUserAndRoles()), context); + return null; + } //TODO(zjffdu) move these to Notebook if (StringUtils.isBlank(newNotePath)) { newNotePath = "/Cloned Note_" + noteId; diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java index 85a552e7f45..3c8a0441a76 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java @@ -1160,7 +1160,10 @@ public void onSuccess(String result, ServiceContext context) throws IOException private void cloneNote(NotebookSocket conn, ServiceContext context, Message fromMessage) throws IOException { - String noteId = connectionManager.getAssociatedNoteId(conn); + String noteId = (String) fromMessage.get("id"); + if (noteId == null) { + return; + } String name = (String) fromMessage.get("name"); getNotebookService().cloneNote(noteId, name, context, new WebSocketServiceCallback(conn) { diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookSecurityRestApiTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookSecurityRestApiTest.java index 130ea38270a..4e36b9c80ed 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookSecurityRestApiTest.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookSecurityRestApiTest.java @@ -98,6 +98,25 @@ void testThatOtherUserCannotAccessNoteIfPermissionSet() throws IOException { deleteNoteForUser(noteId, "admin", "password1"); } + @Test + void testThatOtherUserCannotCloneNoteIfPermissionSet() throws IOException { + String noteId = createNoteForUser("test_5", "admin", "password1"); + try { + //set permission + String payload = "{ \"owners\": [\"admin\"], \"readers\": [\"user2\"], " + + "\"runners\": [\"user2\"], \"writers\": [\"user2\"] }"; + CloseableHttpResponse put = httpPut("/notebook/" + noteId + "/permissions", payload , "admin", "password1"); + assertThat("test set note permission method:", put, isAllowed()); + put.close(); + + userTryCloneNote(noteId, "clone_of_test_5", "user1", "password2", isForbidden()); + } finally { + // the notes of this class all land on the same default path, so a leftover note would + // make the next test fail on a name conflict instead of on its own assertion + deleteNoteForUser(noteId, "admin", "password1"); + } + } + @Test void testThatWriterCannotRemoveNote() throws IOException { String noteId = createNoteForUser("test_4", "admin", "password1"); @@ -126,6 +145,14 @@ private void userTryRemoveNote(String noteId, String user, String pwd, delete.close(); } + private void userTryCloneNote(String noteId, String newNoteName, String user, String pwd, + Matcher m) throws IOException { + String jsonRequest = "{\"notePath\":\"" + newNoteName + "\"}"; + CloseableHttpResponse post = httpPost("/notebook/" + noteId, jsonRequest, user, pwd); + assertThat(post, m); + post.close(); + } + private void userTryGetNote(String noteId, String user, String pwd, Matcher m) throws IOException { CloseableHttpResponse get = httpGet("/notebook/" + noteId, user, pwd); diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java index 2eeb0f650c6..46da96e7253 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java @@ -109,8 +109,10 @@ void setUp(TestInfo testInfo) throws Exception { ZeppelinConfiguration zConf = ZeppelinConfiguration.load(); zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_NOTEBOOK_DIR.getVarName(), notebookDir.getAbsolutePath()); - // enable cron for testNoteUpdate method - if ("testNoteUpdate()".equals(testInfo.getDisplayName())){ + // testNoteUpdate needs cron enabled, and both of these need conf/shiro.ini to exist so that + // Zeppelin does not treat the caller as an anonymous deployment + if ("testNoteUpdate()".equals(testInfo.getDisplayName()) + || "testCloneNoteForbiddenWhenShiroIsConfigured()".equals(testInfo.getDisplayName())) { confDir = Files.createTempDirectory("confDir").toAbsolutePath().toFile(); zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_CONF_DIR.getVarName(), confDir.getAbsolutePath()); @@ -474,6 +476,36 @@ void testNoteUpdate() throws IOException { }); } + @Test + void testCloneNoteForbiddenWhenShiroIsConfigured() throws IOException { + String noteId = notebookService.createNote("/clone_protected", "test", true, context, callback); + HashSet otherUser = new HashSet<>(); + otherUser.add("other_user"); + authorizationService.setOwners(noteId, otherUser); + authorizationService.setWriters(noteId, otherUser); + + reset(callback); + assertNull(notebookService.cloneNote(noteId, "/clone_protected_target", context, callback)); + verify(callback).onFailure(any(ForbiddenException.class), eq(context)); + } + + @Test + void testCloneNoteAllowedForEverybodyWithoutShiro() throws IOException { + String noteId = notebookService.createNote("/clone_anonymous", "test", true, context, callback); + HashSet otherUser = new HashSet<>(); + otherUser.add("other_user"); + authorizationService.setOwners(noteId, otherUser); + authorizationService.setWriters(noteId, otherUser); + + // this setup has no conf/shiro.ini, so hasWritePermission grants write access to everybody. + // NotebookRestApi applied that same predicate before delegating here + reset(callback); + String clonedNoteId = + notebookService.cloneNote(noteId, "/clone_anonymous_target", context, callback); + assertNotNull(clonedNoteId); + verify(callback).onSuccess(any(Note.class), eq(context)); + } + @Test void testRenameNoteRejectsDuplicate() throws IOException { String note1Id = notebookService.createNote("/folder/note1", "test", true, context, callback); diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java index 325cb9ee856..d288851fbff 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java @@ -41,11 +41,13 @@ import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.Callable; +import java.util.stream.Collectors; import org.apache.commons.io.IOUtils; import org.apache.thrift.TException; @@ -906,6 +908,88 @@ void testRuntimeInfos() throws IOException { }); } + @Test + void testCloneNoteClonesTheRequestedSourceNote() throws IOException { + String sourceNoteId = null; + String associatedNoteId = null; + String clonedNoteId = null; + + try { + sourceNoteId = notebook.createNote("/clone_source", anonymous); + notebook.processNote(sourceNoteId, + note -> { + Paragraph paragraph = note.addNewParagraph(anonymous); + paragraph.setText("%md source note"); + paragraph.setAuthenticationInfo(anonymous); + notebook.saveNote(note, anonymous); + return null; + }); + associatedNoteId = notebook.createNote("/clone_associated", anonymous); + + NotebookSocket sock = createWebSocket(); + // the socket is looking at another note, which used to decide what CLONE_NOTE copied + notebookServer.onMessage(sock, new Message(OP.GET_NOTE).put("id", associatedNoteId).toJson()); + + String clonedNotePath = "/clone_target_" + System.currentTimeMillis(); + notebookServer.onMessage(sock, new Message(OP.CLONE_NOTE) + .put("id", sourceNoteId) + .put("name", clonedNotePath) + .toJson()); + + clonedNoteId = notebook.getNoteIdByPath(clonedNotePath); + assertNotNull(clonedNoteId, "the requested source note should have been cloned"); + List clonedTexts = notebook.processNote(clonedNoteId, + note -> note.getParagraphs().stream().map(Paragraph::getText).collect(Collectors.toList())); + assertEquals(Collections.singletonList("%md source note"), clonedTexts, + "CLONE_NOTE should copy the note in the request, not the one the socket is looking at"); + } finally { + for (String noteId : new String[] {clonedNoteId, associatedNoteId, sourceNoteId}) { + if (noteId != null) { + notebook.removeNote(noteId, anonymous); + } + } + } + } + + @Test + void testCloneNoteAppliesTheWriteRuleOfTheRestEndpoint() throws IOException { + String sourceNoteId = null; + String associatedNoteId = null; + String clonedNoteId = null; + + try { + sourceNoteId = notebook.createNote("/clone_protected_source", anonymous); + authorizationService.setOwners(sourceNoteId, new HashSet<>(Arrays.asList("someone_else"))); + authorizationService.setWriters(sourceNoteId, new HashSet<>(Arrays.asList("someone_else"))); + associatedNoteId = notebook.createNote("/clone_protected_associated", anonymous); + + NotebookSocket sock = createWebSocket(); + notebookServer.onMessage(sock, new Message(OP.GET_NOTE).put("id", associatedNoteId).toJson()); + + String clonedNotePath = "/clone_protected_target_" + System.currentTimeMillis(); + notebookServer.onMessage(sock, new Message(OP.CLONE_NOTE) + .put("id", sourceNoteId) + .put("name", clonedNotePath) + .toJson()); + + // this server has no conf/shiro.ini, so hasWritePermission grants write access to everybody + // and the REST clone endpoint accepts this note. The WebSocket path has to accept it too + clonedNoteId = notebook.getNoteIdByPath(clonedNotePath); + assertNotNull(clonedNoteId, + "an explicit ACL must not block the clone while Zeppelin runs in anonymous mode"); + } finally { + if (sourceNoteId != null) { + authorizationService.setOwners(sourceNoteId, new HashSet<>()); + authorizationService.setWriters(sourceNoteId, new HashSet<>()); + } + for (String noteId : new String[] {clonedNoteId, associatedNoteId, sourceNoteId}) { + if (noteId != null) { + notebook.removeNote(noteId, anonymous); + } + } + } + } + @Test void testGetParagraphList() throws IOException { String noteId = null;