New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ZEPPELIN-5934] Check notebook folder permissions before allowing to rename, remove or restore it #4624
base: master
Are you sure you want to change the base?
[ZEPPELIN-5934] Check notebook folder permissions before allowing to rename, remove or restore it #4624
Conversation
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. |
Issues in my last comment were resolved in the latest commit. |
try { | ||
NoteManager.Folder folder = notebook.getFolder("/" + folderPath); | ||
if (!checkFolderPermission(folder, Permission.OWNER, Message.OP.MOVE_FOLDER_TO_TRASH, context, callback)) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before returning, shouldn't we call callback.onFailure
?
permission, noteInfo.getNoteName(), | ||
allowed, | ||
context.getAutheInfo().getUser(), context.getUserAndRoles()); | ||
callback.onFailure(new ForbiddenException(errorMsg), context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution. It looks good to me except for the minor comments. Could you please address the comment?
BTW, there are no tests for the features. Could you please add tests for your feature? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not a good idea to add the logic regarding the rights in the service. Furthermore, information on Notes are leaked, where a user may not have access at all.
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
zeppelin/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java
Lines 230 to 232 in 55a614a
if (!isNotePathAvailable(newNotePath)) { | |
throw new NotePathAlreadyExistsException("Note '" + newNotePath + "' existed"); | |
} |
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?
How should this be tested?
NotebookServiceTest
or inNoteManagerTest
to cover multi-user scenarios.