You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by "idzikovsky (via GitHub)" <gi...@apache.org> on 2023/06/22 14:49:21 UTC
[GitHub] [zeppelin] idzikovsky opened a new pull request, #4624: [ZEPPELIN-5934] Check notebook folder permissions before allowing to rename, remove or restore it
idzikovsky opened a new pull request, #4624:
URL: https://github.com/apache/zeppelin/pull/4624
### What is this PR for?
Users who are able to see notes in some directory can rename, move to trash and remove from trash that directory without being owner or having write permissions for notes in that directory.
This is resolved in first commit.
Secondly, after renaming directory to the name of already existing directory, old notes in target directory become unaccessible (in fact those notes are removed in file system, but still visible in UI as they are present in NoteManager registry).
This addresses 2nd point in ZEPPELIN-5333.
Fixed in 2nd commit.
### What type of PR is it?
Improvement
### What is the Jira issue?
* [ZEPPELIN-5934](https://issues.apache.org/jira/browse/ZEPPELIN-5934)
* [ZEPPELIN-5333.](https://issues.apache.org/jira/browse/ZEPPELIN-5333.)
### How should this be tested?
* I've tested this manually by creating some note in directory, giving read permission to it for some other user, and checking whether that user can move to trash, remove, rename or restore directory with that note.
* I've checked and it does not seem like there are available infrastructure in `NotebookServiceTest` or in `NoteManagerTest` to cover multi-user scenarios.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on pull request #4624: [ZEPPELIN-5934] Check notebook folder permissions before allowing to rename, remove or restore it
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul commented on PR #4624:
URL: https://github.com/apache/zeppelin/pull/4624#issuecomment-1610385548
BTW, there are no tests for the features. Could you please add tests for your feature?
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4624: [ZEPPELIN-5934] Check notebook folder permissions before allowing to rename, remove or restore it
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul commented on code in PR #4624:
URL: https://github.com/apache/zeppelin/pull/4624#discussion_r1244498036
##########
zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java:
##########
@@ -1192,24 +1203,38 @@ public void moveFolderToTrash(String folderPath,
ServiceContext context,
ServiceCallback<Void> 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 destFolderPath = "/" + NoteManager.TRASH_FOLDER + "/" + folderPath;
- if (notebook.containsNote(destFolderPath)) {
- destFolderPath = destFolderPath + " " +
- TRASH_CONFLICT_TIMESTAMP_FORMATTER.format(Instant.now());
- }
+ try {
+ NoteManager.Folder folder = notebook.getFolder("/" + folderPath);
+ if (!checkFolderPermission(folder, Permission.OWNER, Message.OP.MOVE_FOLDER_TO_TRASH, context, callback)) {
+ return;
Review Comment:
before returning, shouldn't we call `callback.onFailure`?
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] idzikovsky commented on pull request #4624: [ZEPPELIN-5934] Check notebook folder permissions before allowing to rename, remove or restore it
Posted by "idzikovsky (via GitHub)" <gi...@apache.org>.
idzikovsky commented on PR #4624:
URL: https://github.com/apache/zeppelin/pull/4624#issuecomment-1609093390
Issues in my last comment were resolved in the latest commit.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] idzikovsky commented on pull request #4624: [ZEPPELIN-5934] Check notebook folder permissions before allowing to rename, remove or restore it
Posted by "idzikovsky (via GitHub)" <gi...@apache.org>.
idzikovsky commented on PR #4624:
URL: https://github.com/apache/zeppelin/pull/4624#issuecomment-1605038003
Sorry, I made this PR on top of 0.10.1 version, and it seems like something has been changed in this filed in master branch since then.
Now I have local fix of this issue for master branch. I'll update the PR as soon as I have successful run of tests.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4624: [ZEPPELIN-5934] Check notebook folder permissions before allowing to rename, remove or restore it
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul commented on code in PR #4624:
URL: https://github.com/apache/zeppelin/pull/4624#discussion_r1244500227
##########
zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java:
##########
@@ -1497,4 +1530,78 @@ private <T> boolean checkPermission(String noteId,
return false;
}
}
+
+ /**
+ * Return null when it is allowed, otherwise return the error message which could be
+ * propagated to frontend
+ *
+ * @param noteInfo
+ * @param context
+ * @param permission
+ * @param op
+ * @return
+ */
+ private <T> boolean checkPermission(NoteInfo noteInfo,
+ Permission permission,
+ Message.OP op,
+ ServiceContext context,
+ ServiceCallback<T> callback) throws IOException {
+ boolean isAllowed = false;
+ Set<String> allowed = null;
+ switch (permission) {
+ case READER:
+ isAllowed = authorizationService.isReader(noteInfo.getId(), context.getUserAndRoles());
+ allowed = authorizationService.getReaders(noteInfo.getId());
+ break;
+ case WRITER:
+ isAllowed = authorizationService.isWriter(noteInfo.getId(), context.getUserAndRoles());
+ allowed = authorizationService.getWriters(noteInfo.getId());
+ break;
+ case RUNNER:
+ isAllowed = authorizationService.isRunner(noteInfo.getId(), context.getUserAndRoles());
+ allowed = authorizationService.getRunners(noteInfo.getId());
+ break;
+ case OWNER:
+ isAllowed = authorizationService.isOwner(noteInfo.getId(), context.getUserAndRoles());
+ allowed = authorizationService.getOwners(noteInfo.getId());
+ break;
+ }
+ if (isAllowed) {
+ return true;
+ } else {
+ String errorMsg = String.format(
+ "Insufficient %s privileges to '%s' note.\n" +
+ "Allowed users or roles: %s\n" +
+ "But the user %s belongs to: %s",
+ permission, noteInfo.getNoteName(),
+ allowed,
+ context.getAutheInfo().getUser(), context.getUserAndRoles());
+ callback.onFailure(new ForbiddenException(errorMsg), context);
Review Comment:
Oh, I see this code but we'd better move this code outside of this method because it's only adopted when failure. I think it would be good to make it clear the purpose of this method and `callbak.onFailure` doesn't seem to be the purpose of this method.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a diff in pull request #4624: [ZEPPELIN-5934] Check notebook folder permissions before allowing to rename, remove or restore it
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on code in PR #4624:
URL: https://github.com/apache/zeppelin/pull/4624#discussion_r1244808864
##########
zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java:
##########
@@ -376,7 +381,7 @@ private NoteNode getNoteNode(String notePath) throws IOException {
return noteNode;
}
- private Folder getFolder(String folderPath) throws IOException {
+ public Folder getFolder(String folderPath) throws IOException {
Review Comment:
I don't think it's a good idea to give the folder object to the outside. Do you see a way to do the permissions check in the NoteManager class?
Currently there are already some checks in the NoteManager class.
https://github.com/apache/zeppelin/blob/55a614ad16d220842324f425a159cf41612d76f6/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java#L230-L232
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org