From 8784ffab9e8aafe185cd48cc79952fd2607ea596 Mon Sep 17 00:00:00 2001 From: renechoi Date: Sun, 30 Aug 2026 00:01:51 +0900 Subject: [PATCH] [ZEPPELIN-6684] Clone the requested note and check writer access The WebSocket CLONE_NOTE handler cloned the note the socket was associated with and ignored the id the client sent, so it could copy a different note than the one requested. It also skipped the writer check that the REST clone endpoint runs against its explicit source note. Take the source note from the request, and move the writer check into NotebookService.cloneNote. It uses hasWritePermission, the predicate NotebookRestApi already applied, rather than checkPermission: the two differ when conf/shiro.ini is absent, where hasWritePermission grants write access to everybody, so checkPermission would have refused WebSocket clones that REST still accepts. NotebookRestApi.cloneNote drops its own call instead of checking the same rule twice, and NotebookSecurityRestApiTest covers that the endpoint still answers 403 to a user who cannot write the source note. Both bundled clients already send the id in CLONE_NOTE. --- .../apache/zeppelin/rest/NotebookRestApi.java | 1 - .../zeppelin/service/NotebookService.java | 10 +++ .../zeppelin/socket/NotebookServer.java | 5 +- .../rest/NotebookSecurityRestApiTest.java | 27 ++++++ .../zeppelin/service/NotebookServiceTest.java | 36 +++++++- .../zeppelin/socket/NotebookServerTest.java | 84 +++++++++++++++++++ 6 files changed, 159 insertions(+), 4 deletions(-) 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;