This is an automated email from the ASF dual-hosted git repository.
tbonelee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new 113ca83337 [ZEPPELIN-6684] Clone the requested note and check writer
access
113ca83337 is described below
commit 113ca8333727de07214746c55fab1c03f4cd10b4
Author: renechoi <[email protected]>
AuthorDate: Thu Sep 10 00:54:03 2026 +0900
[ZEPPELIN-6684] Clone the requested note and check writer access
### What is this PR for?
`NotebookServer.cloneNote` ignored the `id` in the message and cloned the
socket's note, skipping the writer check REST applies. It now clones the
requested note, and that check moved from REST into
`NotebookService.cloneNote`, keeping REST's `hasWritePermission`.
Correction: revision 1 said the paths shared one rule. They did not. Unlike
`checkPermission`, `hasWritePermission` allows everyone when `conf/shiro.ini`
is absent, so it refused anonymous clones REST allows.
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6684
### How should this be tested?
`NotebookServiceTest`: forbidden with shiro.ini, allowed without.
`NotebookServerTest`: the requested note is cloned.
`NotebookSecurityRestApiTest`: REST still 403s non-writers.
### Questions:
No license or doc change.
Closes #5443 from renechoi/ZEPPELIN-6684.
Signed-off-by: ChanHo Lee <[email protected]>
---
.../org/apache/zeppelin/rest/NotebookRestApi.java | 1 -
.../apache/zeppelin/service/NotebookService.java | 10 +++
.../org/apache/zeppelin/socket/NotebookServer.java | 5 +-
.../zeppelin/rest/NotebookSecurityRestApiTest.java | 27 +++++++
.../zeppelin/service/NotebookServiceTest.java | 36 +++++++++-
.../apache/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 3c09a612f4..0df06056e8 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 class NotebookRestApi extends AbstractRestApi {
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 eacd970eb0..a0e7eb592f 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 class NotebookService {
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;
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 6ab2ef711e..fcf76a8105 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
@@ -1222,7 +1222,10 @@ public class NotebookServer implements
AngularObjectRegistryListener,
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) {
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 130ea38270..4e36b9c80e 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 @@ public class NotebookSecurityRestApiTest extends
AbstractTestRestApi {
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 @@ public class NotebookSecurityRestApiTest extends
AbstractTestRestApi {
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);
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 1c4f554d50..549b6299b8 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
@@ -111,9 +111,11 @@ class NotebookServiceTest {
zConf = ZeppelinConfiguration.load();
zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_NOTEBOOK_DIR.getVarName(),
notebookDir.getAbsolutePath());
- // enable cron for the tests that update a note's cron settings
+ // The cron tests need cron enabled, and the protected clone test needs
conf/shiro.ini so that
+ // Zeppelin does not treat the caller as an anonymous deployment.
if ("testNoteUpdate()".equals(testInfo.getDisplayName())
- ||
"testCronRefreshedOnlyWhenTheExpressionChanges()".equals(testInfo.getDisplayName()))
{
+ ||
"testCronRefreshedOnlyWhenTheExpressionChanges()".equals(testInfo.getDisplayName())
+ ||
"testCloneNoteForbiddenWhenShiroIsConfigured()".equals(testInfo.getDisplayName()))
{
confDir = Files.createTempDirectory("confDir").toAbsolutePath().toFile();
zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_CONF_DIR.getVarName(),
confDir.getAbsolutePath());
@@ -519,6 +521,36 @@ class NotebookServiceTest {
verify(schedulerService).refreshCron(noteId);
}
+ @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);
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 325cb9ee85..d288851fbf 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.nio.charset.StandardCharsets;
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 @@ class NotebookServerTest extends AbstractTestRestApi {
});
}
+ @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;
