[ZEPPELIN-6684] Clone the requested note and check writer access

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;

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<Note> 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;

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<Note>(conn) {

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<? super CloseableHttpResponse> 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<? super CloseableHttpResponse> m) throws IOException {
     CloseableHttpResponse get = httpGet("/notebook/" + noteId, user, pwd);

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<String> 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<String> 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);

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<String> 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;