You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "sylee6529 (via GitHub)" <gi...@apache.org> on 2026/08/05 02:45:34 UTC
[PR] [ZEPPELIN-5934] Check folder permissions before rename, trash and remove [zeppelin]
sylee6529 opened a new pull request, #5383:
URL: https://github.com/apache/zeppelin/pull/5383
### What is this PR for?
Folder level operations in `NotebookService` ran without any permission check. `renameFolder` and `moveFolderToTrash` carried a `//TODO(zjffdu) folder permission check`, and `removeFolder` and `restoreFolder` had no check at all. A user who could only read one note in a folder was able to rename, trash or permanently delete that whole folder, even though the same user could not touch the note itself.
**Deriving a folder permission.** Zeppelin has no folder level ACL. `AuthorizationService` is keyed by note id only and the word "folder" does not appear in it; a folder is just the in-memory tree `NoteManager` builds out of note paths. So `checkFolderPermission` derives the permission of a folder from the notes it holds, walked recursively, and allows the operation only when the caller holds the required permission on every one of them. No new ACL concept, no storage or migration change.
**Permission levels follow the note level policy they mirror**, so this PR extends an existing rule rather than inventing one:
| Folder operation | Level | Note level counterpart |
| --- | --- | --- |
| `FOLDER_RENAME` | OWNER | `NOTE_RENAME` = OWNER |
| `MOVE_FOLDER_TO_TRASH` | OWNER | `MOVE_NOTE_TO_TRASH` = OWNER |
| `REMOVE_FOLDER` | OWNER | `DEL_NOTE` = OWNER |
| `RESTORE_FOLDER` | **WRITER** | `RESTORE_NOTE` = WRITER |
Restore being lower than trash looks asymmetric, but it is exactly the asymmetry the note level policy already has.
**All-or-nothing.** The check completes before any repository call, so a refused operation leaves the folder exactly as it was instead of half applied. Applying a move or a remove only to the subset the caller owns would leave a partially emptied folder that the caller can neither see nor undo.
**A folder holding no note is allowed.** Removing the last note leaves the folder node in the tree, and there is nothing left to protect. Refusing instead would leave empty folders nobody can ever clean up.
**What the error message reveals.** It names the folder and how many notes blocked the call, but not which ones, since the caller may not be allowed to read them. Those paths go to the server log instead. This is the same split `NotebookRestApi.ownerPermissionError` and `NotebookServer.permissionError` already use, and it matches the existing note level message, which also does not name the note. It is the one place the folder message deliberately differs from the note one: it omits `Allowed users or roles`, because a folder has many notes and the union of their owners would leak exactly what the message is trying to withhold.
`checkPermission` was split into `hasPermission` and `getAllowedEntities` so the folder check can reuse the level dispatch as a plain predicate. The user facing message and behaviour of the note level check are unchanged.
**Relation to earlier work.** PR #4624 (ZEPPELIN-5934, closed by the stale bot after inactivity, never merged) took the same "derive from the notes" approach, and this PR keeps that idea. It differs in four ways: `restoreFolder` is WRITER rather than OWNER so it lines up with `RESTORE_NOTE`; `NoteManager.getFolder` stays private, since the already public `getNoteInfoRecursively` is enough and `NoteManager.Folder` need not reach the service layer; the unrelated `FolderPathAlreadyExistsException` commit is left out; and the tests the reviewer asked for are included.
### Scope
Limited to the four operations that act on one named folder. `EMPTY_TRASH` and `RESTORE_ALL` act on the shared trash as a whole and are untouched, on purpose.
`emptyTrash` does destroy trashed notes without a check, so it can still remove a folder that `removeFolder` now refuses. That gap is not introduced here: `removeNote` already requires OWNER for a single note today, and `emptyTrash` already bypasses it. It belongs to note level enforcement, and deciding who may empty a trash that every user shares is a product question worth its own issue rather than a rider on this fix. Happy to file it as a follow-up.
Two adjacent issues are also left alone: `renameFolder` checks the source folder but not the destination, so renaming onto an existing folder path is still unguarded (ZEPPELIN-5333), and `moveFolderToTrash` detects trash name conflicts with `containsNote` on a folder path, which never matches.
### Known limitation
The check and the operation are not atomic. If another user adds a note to the folder between them, that note is included in the operation without having been checked. Every existing note level `checkPermission` call has the same shape, so closing it would mean introducing locking across the whole permission layer rather than in this path alone.
### What type of PR is it?
Bug Fix
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5934
### How should this be tested?
Automated. `mvn test -pl zeppelin-server -Dtest='NotebookServiceTest,NotebookServerTest,NotebookSecurityRestApiTest'`
`NotebookServiceTest`
* `testFolderOperationsRequirePermissionOnEveryNote` — a folder holds two notes, the caller owns one of them, and the blocking note sits in a sub folder so the check is only reached by walking recursively. `renameFolder`, `moveFolderToTrash` and `removeFolder` are all refused with a `ForbiddenException`, both notes are still at their original paths afterwards, and the message names the folder but not the blocking note. The owner of both notes then performs the same operations successfully.
* `testRestoreFolderRequiresWriterPermission` — a reader is refused, a writer succeeds and the note is back at its original path.
* `testRemoveFolderHoldingNoNoteIsAllowed` — pins the empty folder decision.
`NotebookServerTest#testRemoveFolderRequiresOwnerOnEveryNote` covers the real entry point. There is no REST endpoint for folder operations; every caller of these service methods is in `NotebookServer`, so this drives a real `MiniZeppelinServer` with an actual `REMOVE_FOLDER` WebSocket message carrying a `TicketContainer` principal. It asserts the notes survive and the client receives `AUTH_INFO`, then that the same message succeeds once the caller owns everything.
Without the production change, two of the `NotebookServiceTest` cases and the `NotebookServerTest` case fail; the WebSocket one fails on the folder actually having been deleted by a non-owner.
`NotebookSecurityRestApiTest` (a real server with Shiro over HTTP) passes unchanged, covering the `checkPermission` split.
Manual: with two users, give user A owner of a note inside a folder and leave user B as reader only, then have B rename, trash and delete the folder from the notebook list. Each is refused and the folder stays.
### Questions
* Does the license files need to update? No.
* Is there breaking changes for older versions? Behaviour changes on purpose: folder rename, trash, remove and restore now fail for users who lack the permission on every note in the folder, where they previously succeeded. Deployments that relied on the missing check will see refusals. No API, config or storage format change.
* Does this needs documentation? No. It brings folder operations in line with the note permission model the docs already describe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-5934] Check folder permissions before rename, trash and remove [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on code in PR #5383:
URL: https://github.com/apache/zeppelin/pull/5383#discussion_r3889542773
##########
zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java:
##########
@@ -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<NoteInfo> 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<NoteInfo> notesInfo = notebookService.removeFolder("/folder_1", user2, callback);
+ verify(callback).onSuccess(notesInfo, user2);
+ assertEquals(0, notesInfo.size());
Review Comment:
Same shape here. `notesInfo` is not the folder's contents, it is every note user2 can read, and the only note was removed just above, so it is 0 whether or not the folder was actually deleted. Removing the `notebook.removeFolder(...)` call from `removeFolder` also leaves this test passing.
Since what this test pins down is that a folder holding no note can be removed by anyone, checking the folder itself would say it:
```java
assertFalse(notebook.containsFolder("/folder_1"));
```
##########
zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java:
##########
@@ -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<NoteInfo> 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());
Review Comment:
This assertion passes whether or not the restore actually happened. I removed the `notebook.moveFolder(...)` call from `restoreFolder` entirely and this test stayed green.
`NoteManager.moveFolder` only updates the tree's `notesInfo` and never touches the note objects, unlike `moveNote`, which does `note.setPath()` + `saveNote()`. So after the folder is moved to trash, `Note.getPath()` still returns `/Backup/note1`, while the tree correctly reports `/~Trash/Backup/note1` at the same moment.
Asking the tree catches that mutation:
```java
assertTrue(notebook.containsNote("/Backup/note1"));
assertFalse(notebook.containsNote("/~Trash/Backup/note1"));
```
It also drops the `getNote` call, so the test gets shorter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org