From 86266de76c656561a74f55483ca34811c59dae95 Mon Sep 17 00:00:00 2001 From: Seoyeon Lee <68765200+sylee6529@users.noreply.github.com> Date: Wed, 5 Aug 2026 11:19:51 +0900 Subject: [PATCH] [ZEPPELIN-5934] Check folder permissions before rename, trash and remove NotebookService applied folder level operations without looking at permissions at all: renameFolder and moveFolderToTrash carried a "TODO(zjffdu) folder permission check", and removeFolder and restoreFolder had no check to begin with. A user who could only read a note was able to rename, trash or permanently delete the whole folder holding it, even though that same user could not touch the note itself. Zeppelin has no folder level ACL. A folder is only the in-memory tree NoteManager builds out of note paths, so checkFolderPermission derives the permission of a folder from the notes under it, recursively, and allows the operation only when the caller holds the required permission on every one of them. The levels follow the note level policy they mirror: OWNER for rename, trash and remove, matching NOTE_RENAME, MOVE_NOTE_TO_TRASH and DEL_NOTE, and WRITER for restore, matching RESTORE_NOTE. The check is all-or-nothing and runs before any repository call, so a refused operation leaves the folder exactly as it was rather than half applied. The error message names the folder and how many notes blocked the call, but not which ones, because the caller may not be allowed to read them; those paths go to the server log instead, the same split NotebookRestApi.ownerPermissionError already uses. Scope is the four operations that act on one named folder. EMPTY_TRASH and RESTORE_ALL act on the shared trash as a whole and stay untouched: emptyTrash already bypasses the OWNER check that removeNote performs on a single note, so that gap belongs to note level enforcement and needs its own decision about who may empty a trash that every user shares. --- .../apache/zeppelin/notebook/Notebook.java | 4 + .../zeppelin/service/NotebookService.java | 115 +++++++++++++---- .../zeppelin/service/NotebookServiceTest.java | 116 +++++++++++++++++- .../zeppelin/socket/NotebookServerTest.java | 51 ++++++++ 4 files changed, 257 insertions(+), 29 deletions(-) diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/Notebook.java b/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/Notebook.java index 83f0032822f..472842c44ab 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/Notebook.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/Notebook.java @@ -540,6 +540,10 @@ public boolean containsFolder(String folderPath) { return noteManager.containsFolder(folderPath); } + public List getNoteInfoRecursively(String folderPath) throws IOException { + return noteManager.getNoteInfoRecursively(folderPath); + } + public void moveNote(String noteId, String newNotePath, AuthenticationInfo subject) throws IOException { LOGGER.info("Move note {} to {}", noteId, newNotePath); noteManager.moveNote(noteId, newNotePath, subject); 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 554b85f4de2..a13018e1d0d 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 @@ -715,6 +715,11 @@ public void restoreFolder(String folderPath, return; } try { + // RESTORE_NOTE only asks for WRITER, so restoring a whole folder stays on the same level. + if (!checkFolderPermission(folderPath, Permission.WRITER, Message.OP.RESTORE_FOLDER, + context, callback)) { + return; + } String destFolderPath = folderPath.replace("/" + NoteManager.TRASH_FOLDER, ""); notebook.moveFolder(folderPath, destFolderPath, context.getAutheInfo()); callback.onSuccess(null, context); @@ -1261,17 +1266,22 @@ public void moveFolderToTrash(String folderPath, ServiceContext context, ServiceCallback callback) throws IOException { - //TODO(zjffdu) folder permission check //TODO(zjffdu) folderPath is relative path, need to fix it in frontend LOGGER.info("Move folder {} to trash", folderPath); + String srcFolderPath = "/" + folderPath; + if (!checkFolderPermission(srcFolderPath, Permission.OWNER, + Message.OP.MOVE_FOLDER_TO_TRASH, context, callback)) { + return; + } + String destFolderPath = "/" + NoteManager.TRASH_FOLDER + "/" + folderPath; if (notebook.containsNote(destFolderPath)) { destFolderPath = destFolderPath + " " + TRASH_CONFLICT_TIMESTAMP_FORMATTER.format(Instant.now()); } - notebook.moveFolder("/" + folderPath, destFolderPath, context.getAutheInfo()); + notebook.moveFolder(srcFolderPath, destFolderPath, context.getAutheInfo()); callback.onSuccess(null, context); } @@ -1291,6 +1301,10 @@ public List removeFolder(String folderPath, ServiceContext context, ServiceCallback> callback) throws IOException { try { + if (!checkFolderPermission(folderPath, Permission.OWNER, Message.OP.REMOVE_FOLDER, + context, callback)) { + return null; + } notebook.removeFolder(folderPath, context.getAutheInfo()); List notesInfo = notebook.getNotesInfo( noteId -> authorizationService.isReader(noteId, context.getUserAndRoles())); @@ -1306,10 +1320,13 @@ public List renameFolder(String folderPath, String newFolderPath, ServiceContext context, ServiceCallback> callback) throws IOException { - //TODO(zjffdu) folder permission check - try { - notebook.moveFolder(normalizeNotePath(folderPath), + String normalizedFolderPath = normalizeNotePath(folderPath); + if (!checkFolderPermission(normalizedFolderPath, Permission.OWNER, + Message.OP.FOLDER_RENAME, context, callback)) { + return null; + } + notebook.moveFolder(normalizedFolderPath, normalizeNotePath(newFolderPath), context.getAutheInfo()); List notesInfo = notebook.getNotesInfo( noteId -> authorizationService.isReader(noteId, context.getUserAndRoles())); @@ -1536,34 +1553,78 @@ private boolean checkPermission(String noteId, Message.OP op, ServiceContext context, ServiceCallback callback) throws IOException { - boolean isAllowed = false; - Set allowed = null; + if (hasPermission(noteId, permission, context.getUserAndRoles())) { + return true; + } else { + String errorMsg = "Insufficient privileges to " + permission + " note.\n" + + "Allowed users or roles: " + getAllowedEntities(noteId, permission) + "\n" + + "But the user " + context.getAutheInfo().getUser() + + " belongs to: " + context.getUserAndRoles(); + callback.onFailure(new ForbiddenException(errorMsg), context); + return false; + } + } + + /** + * Zeppelin has no folder level ACL, so the permission of a folder is derived from the notes + * under it: the operation is allowed only when the caller holds the required permission on + * every one of them, and a folder holding no note is allowed. + * + * @return true when the operation may proceed, false after the callback has been failed + */ + private boolean checkFolderPermission(String folderPath, + Permission permission, + Message.OP op, + ServiceContext context, + ServiceCallback callback) throws IOException { + List deniedNotePaths = new ArrayList<>(); + for (NoteInfo noteInfo : notebook.getNoteInfoRecursively(folderPath)) { + if (!hasPermission(noteInfo.getId(), permission, context.getUserAndRoles())) { + deniedNotePaths.add(noteInfo.getPath()); + } + } + if (deniedNotePaths.isEmpty()) { + return true; + } else { + // Denied paths go to the log only: the caller may not be allowed to read those notes. + LOGGER.info("Permission check failed for {} on folder {}, user {} lacks {} on {}", + op, folderPath, context.getAutheInfo().getUser(), permission, deniedNotePaths); + String errorMsg = "Insufficient privileges to " + permission + " folder " + folderPath + + ".\n" + deniedNotePaths.size() + " of the notes it holds require " + permission + + " privileges.\n" + "But the user " + context.getAutheInfo().getUser() + + " belongs to: " + context.getUserAndRoles(); + callback.onFailure(new ForbiddenException(errorMsg), context); + return false; + } + } + + private boolean hasPermission(String noteId, Permission permission, Set userAndRoles) { switch (permission) { case READER: - isAllowed = authorizationService.isReader(noteId, context.getUserAndRoles()); - allowed = authorizationService.getReaders(noteId); - break; + return authorizationService.isReader(noteId, userAndRoles); case WRITER: - isAllowed = authorizationService.isWriter(noteId, context.getUserAndRoles()); - allowed = authorizationService.getWriters(noteId); - break; + return authorizationService.isWriter(noteId, userAndRoles); case RUNNER: - isAllowed = authorizationService.isRunner(noteId, context.getUserAndRoles()); - allowed = authorizationService.getRunners(noteId); - break; + return authorizationService.isRunner(noteId, userAndRoles); case OWNER: - isAllowed = authorizationService.isOwner(noteId, context.getUserAndRoles()); - allowed = authorizationService.getOwners(noteId); - break; + return authorizationService.isOwner(noteId, userAndRoles); + default: + return false; } - if (isAllowed) { - return true; - } else { - String errorMsg = "Insufficient privileges to " + permission + " note.\n" + - "Allowed users or roles: " + allowed + "\n" + "But the user " + - context.getAutheInfo().getUser() + " belongs to: " + context.getUserAndRoles(); - callback.onFailure(new ForbiddenException(errorMsg), context); - return false; + } + + private Set getAllowedEntities(String noteId, Permission permission) { + switch (permission) { + case READER: + return authorizationService.getReaders(noteId); + case WRITER: + return authorizationService.getWriters(noteId); + case RUNNER: + return authorizationService.getRunners(noteId); + case OWNER: + return authorizationService.getOwners(noteId); + default: + return Collections.emptySet(); } } 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 0a176ac8b40..ca87b10f593 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 @@ -19,6 +19,7 @@ package org.apache.zeppelin.service; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -36,6 +37,7 @@ import java.io.FileWriter; import java.io.IOException; import java.nio.file.Files; +import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -66,6 +68,7 @@ import org.apache.zeppelin.notebook.repo.NotebookRepo; import org.apache.zeppelin.notebook.repo.VFSNotebookRepo; import org.apache.zeppelin.notebook.scheduler.QuartzSchedulerService; +import org.apache.zeppelin.rest.exception.ForbiddenException; import org.apache.zeppelin.search.LuceneSearch; import org.apache.zeppelin.search.SearchService; import org.apache.zeppelin.storage.ConfigStorage; @@ -88,6 +91,7 @@ class NotebookServiceTest { private File confDir; private SearchService searchService; private Notebook notebook; + private AuthorizationService authorizationService; private ServiceContext context = new ServiceContext(AuthenticationInfo.ANONYMOUS, new HashSet<>()); @@ -136,8 +140,7 @@ void setUp(TestInfo testInfo) throws Exception { when(mockInterpreterSetting.getStatus()).thenReturn(InterpreterSetting.Status.READY); Credentials credentials = new Credentials(); NoteManager noteManager = new NoteManager(notebookRepo, zConf); - AuthorizationService authorizationService = - new AuthorizationService(noteManager, zConf, storage); + authorizationService = new AuthorizationService(noteManager, zConf, storage); notebook = new Notebook( zConf, @@ -410,6 +413,115 @@ void testNoteOperations() throws IOException { assertEquals(0, notesInfo.size()); } + @Test + void testFolderOperationsRequirePermissionOnEveryNote() throws IOException { + ServiceContext user1 = userContext("user1"); + ServiceContext user2 = userContext("user2"); + + // note2 sits in a sub folder, so it is only reached by walking the folder recursively + String note1Id = notebookService.createNote("/folder_1/note1", "test", true, user1, callback); + String note2Id = + notebookService.createNote("/folder_1/nested/note2", "test", true, user1, callback); + // user2 owns one of the two notes + authorizationService.setOwners(note1Id, new HashSet<>(Arrays.asList("user1", "user2"))); + assertTrue(authorizationService.isOwner(note1Id, user2.getUserAndRoles())); + assertFalse(authorizationService.isOwner(note2Id, user2.getUserAndRoles())); + + reset(callback); + assertNull(notebookService.renameFolder("/folder_1", "/folder_2", user2, callback)); + assertForbidden(); + + reset(callback); + notebookService.moveFolderToTrash("folder_1", user2, callback); + assertForbidden(); + + reset(callback); + assertNull(notebookService.removeFolder("/folder_1", user2, callback)); + String errorMsg = assertForbidden(); + // the message names the folder but not the note that blocked the call + assertTrue(errorMsg.contains("/folder_1"), errorMsg); + assertFalse(errorMsg.contains("note2"), errorMsg); + + // nothing was applied + reset(callback); + List notesInfo = notebookService.listNotesInfo(false, user1, callback); + assertEquals(new HashSet<>(Arrays.asList("/folder_1/note1", "/folder_1/nested/note2")), + notesInfo.stream().map(NoteInfo::getPath).collect(Collectors.toSet())); + + // the owner of every note succeeds + reset(callback); + notesInfo = notebookService.renameFolder("/folder_1", "/folder_2", user1, callback); + verify(callback).onSuccess(notesInfo, user1); + assertEquals(new HashSet<>(Arrays.asList("/folder_2/note1", "/folder_2/nested/note2")), + notesInfo.stream().map(NoteInfo::getPath).collect(Collectors.toSet())); + + reset(callback); + notesInfo = notebookService.removeFolder("/folder_2", user1, callback); + verify(callback).onSuccess(notesInfo, user1); + assertEquals(0, notesInfo.size()); + } + + @Test + void testRestoreFolderRequiresWriterPermission() throws IOException { + ServiceContext user1 = userContext("user1"); + ServiceContext user2 = userContext("user2"); + ServiceContext user3 = userContext("user3"); + + String noteId = notebookService.createNote("/Backup/note1", "test", true, user1, callback); + // user2 may write the note, user3 may only read it + authorizationService.setWriters(noteId, new HashSet<>(Arrays.asList("user1", "user2"))); + authorizationService.setReaders(noteId, + new HashSet<>(Arrays.asList("user1", "user2", "user3"))); + assertTrue(authorizationService.isReader(noteId, user3.getUserAndRoles())); + assertFalse(authorizationService.isWriter(noteId, user3.getUserAndRoles())); + assertTrue(authorizationService.isWriter(noteId, user2.getUserAndRoles())); + notebookService.moveFolderToTrash("Backup", user1, callback); + + // a reader is not enough + reset(callback); + notebookService.restoreFolder("/~Trash/Backup", user3, callback); + assertForbidden(); + + // a writer is, like RESTORE_NOTE + reset(callback); + notebookService.restoreFolder("/~Trash/Backup", user2, callback); + verify(callback).onSuccess(null, user2); + notebookService.getNote(noteId, user2, callback, + restoredNote -> { + assertEquals("/Backup/note1", restoredNote.getPath()); + return null; + }); + } + + @Test + void testRemoveFolderHoldingNoNoteIsAllowed() throws IOException { + ServiceContext user1 = userContext("user1"); + ServiceContext user2 = userContext("user2"); + + String noteId = notebookService.createNote("/folder_1/note1", "test", true, user1, callback); + // removing the last note leaves the folder itself in the tree + notebookService.removeNote(noteId, user1, callback); + + reset(callback); + List notesInfo = notebookService.removeFolder("/folder_1", user2, callback); + verify(callback).onSuccess(notesInfo, user2); + assertEquals(0, notesInfo.size()); + } + + private ServiceContext userContext(String user) { + return new ServiceContext(new AuthenticationInfo(user), + new HashSet<>(Arrays.asList(user))); + } + + // returns the message the caller would receive, to check what it does not reveal + private String assertForbidden() throws IOException { + ArgumentCaptor exception = ArgumentCaptor.forClass(Exception.class); + verify(callback).onFailure(exception.capture(), any(ServiceContext.class)); + assertTrue(exception.getValue() instanceof ForbiddenException, + "Expected a ForbiddenException but got: " + exception.getValue()); + return ((ForbiddenException) exception.getValue()).getResponse().getEntity().toString(); + } + @Test void testNoteUpdate() throws IOException { // create note 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 d982d46a33c..ea6e3f3ae19 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 @@ -26,6 +26,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.RETURNS_DEEP_STUBS; import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; @@ -72,11 +73,13 @@ import org.apache.zeppelin.scheduler.Job.Status; import org.apache.zeppelin.service.NotebookService; import org.apache.zeppelin.service.ServiceContext; +import org.apache.zeppelin.ticket.TicketContainer; import org.apache.zeppelin.user.AuthenticationInfo; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import org.mockito.Mockito; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -923,6 +926,54 @@ void testGetParagraphList() throws IOException { } } + @Test + void testRemoveFolderRequiresOwnerOnEveryNote() throws IOException { + String note1Id = null; + String note2Id = null; + try { + note1Id = notebook.createNote("/ws_folder/note1", anonymous); + note2Id = notebook.createNote("/ws_folder/note2", anonymous); + // user1 owns only one of the two notes + authorizationService.setOwners(note1Id, new HashSet<>(Arrays.asList("user1"))); + authorizationService.setOwners(note2Id, new HashSet<>(Arrays.asList("user2"))); + + NotebookSocket sock = createWebSocket(); + notebookServer.onMessage(sock, removeFolderMessage("ws_folder", "user1")); + + // the notes survive and the client is told why + assertTrue(notebook.containsNote("/ws_folder/note1")); + assertTrue(notebook.containsNote("/ws_folder/note2")); + ArgumentCaptor sent = ArgumentCaptor.forClass(String.class); + verify(sock, atLeastOnce()).send(sent.capture()); + assertTrue(sent.getAllValues().stream().anyMatch(m -> m.contains(OP.AUTH_INFO.name())), + "Expected an AUTH_INFO message, but got: " + sent.getAllValues()); + + // the owner of every note succeeds + authorizationService.setOwners(note2Id, new HashSet<>(Arrays.asList("user1"))); + notebookServer.onMessage(createWebSocket(), removeFolderMessage("ws_folder", "user1")); + assertFalse(notebook.containsNote("/ws_folder/note1")); + assertFalse(notebook.containsNote("/ws_folder/note2")); + note1Id = null; + note2Id = null; + } finally { + if (note1Id != null) { + notebook.removeNote(note1Id, anonymous); + } + if (note2Id != null) { + notebook.removeNote(note2Id, anonymous); + } + } + } + + private String removeFolderMessage(String folderPath, String principal) { + String ticket = TicketContainer.instance.getTicket(principal, + new HashSet<>(Arrays.asList(principal))); + Message message = new Message(OP.REMOVE_FOLDER).put("id", folderPath); + message.principal = principal; + message.ticket = ticket; + return message.toJson(); + } + @Test void testNoteRevision() throws IOException { String noteId = null;