-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Added Shiro security #53
Conversation
No need to proceed web resources here.
No cleanup is done, since the same user accross different devices share the same ticket The Map size is at most the number of different user names having access to a Zeppelin instance
…each note is now attached to a user
…tial HTTP call to the SecurityRestApi. This proves that web socket access is done by the user who provided the credentials through Shiro on the HTTP channel.
Tests are updated accordingly by using the anonymous user.
Support for authentication only. |
Hi @hayssams, Thanks for contribution. Let me test this branch. |
Hi @Leemoonsoo, did you get a chance to test this PR ? |
Sorry for late responding and thanks again for contribution. In my understanding, this branch trying to provides 1) security of rest/websocket api. 2) multi-tenant environment for notebook save/load. Because of this PR brings wide range of changes, we have couple of things to discuss.
I'm feeling like if it is possible to separate 1) security of rest/websocket 2) multi-tenancy of notebook into different issue, it'll be much faster to review and test. What do you think? |
You're right, this PR addresses 1) & 2). Regarding the points you raised
Actually feature 2) involves a very small amount of code (load/save notes in user directory). I am personally interested by feature 2). let me know if you want me to complete the other points you raised first or if you prefer to stick to feature 1). I'll respect your choice. |
Hi, For me I am really confuse about the change on the rest api, why do we need |
Hi Anthony
|
I missed your last question about session handling. |
* master: #64 Force opacity 0 in .ace_text-input.ace_composition Fix process creation behavior ZEPPELIN-59 fix interpreter restart Fixes a typo in conf_dir check ZEPPELIN-26 Pluggable notebook persistence layer Conflicts: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java
I think we should have an organized plan to review and address this PR. I propose that the PR be merged into a new branch of Zeppelin, where the community as a whole will have an easier time reviewing and making PRs on the branch. Then I propose that we review the aspects of this separately. By reviewing/approving/voting-on each aspect in sequence, we will have a clear idea of where the PR is in process, and that will make it easier to move PR forward to final merge (if that's what we want to do). The aspects I see are:
The reason I propose a more formal process is that this PR has been sitting here for 8 months now. With that, and with some 82 (!!!) commits, a more structured approach seems like the best way to resolve this PR (one way or the other). (In full disclosure -- My only view about this is that since its security-related, it needs to have mounds of tests to make sure the security doesn't get compromised by later commits.) |
…into shira-security # Conflicts: # zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java # zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java # zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java # zeppelin-web/src/components/navbar/navbar.controller.js # zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java # zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java # zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java
In addition to @elbamos's comments, I hope that the community would understand that it's too hard to review a kind of this PR which has changed widely because committers may not know features well. I, however, also know some features take extensive changes. In this case, I propose that a contributor makes a feature Jira issue that includes several sub-tasks, and makes a - relatively - small PR for every sub-task. It would help review, test and merge the PR easily. |
@jongyoul @elbamos @hayssams this change is an important one, and we should try to get this in. My two cents on how we could divide the whole pull request into:
If the committers can provide some bandwidth for review, I'm willing to make the above changes + documentation. As side notes:
I'm of the opinion that Shiro is a good strategy, larger projects such as https://knox.apache.org/ use Shiro, for HDFS Api gateway security. |
…ed user are returned.
…into shira-security
@hayssams Sure! You can split this into several commits because this PR is your contribution and we will thank you so much of that. If you want to do this, would you mind creating Jira issue or linking a related issue that is already created? |
Is there an example of how shiro would be pluggable with other forms of
|
@hayssams please do that, it will be great. I can help with Junits, Docs and separation, should you need it, will commit into your branches, which can then be submitted as PRs. |
@jongyoul @elbamos @rconline
Once we decide how to move on, I think that we will be able to focus on the implementation details and how this PR should be split. Waiting to hear from you. |
@hayssams Can you provide a very step-by-step intro readme? Default username/pass, what the authentication backend is, how users/groups are added, how to make notebooks belong to one user or another... That very basic stuff? |
@elbamos @jongyoul @rconline For easier review, this new PR (#586) contains only two commits. The first one implements HTTP Auth and the second one Websocket authentication . Once PR # 586 is merged / validated, I suggest that we move forward with note ownership and permissions. |
One comment: why is Stormpath (a commercial auth service unrelated to Zeppelin) mentioned in the comments for this PR? From: Hayssam Saleh @elbamos @jongyoul @rconline For easier review, this new PR (#586) contains only two commits. The first one implements HTTP Auth and the second one Websocket authentication . Once PR # 586 is merged / validated, I suggest that we move forward with note ownership and permissions. — |
You are right that should be removed.
EBIZNEXT http://www.ebiznext.com Java / .NET */ DevOps / BigData / UX |
@jongyoul |
I am closing this PR and suggest that we move the discussion to #586 |
Added Authentication. Once authenticated, a user has access to all notes. HTTP & Websocket channels are secured and require auth. This PR is based on #53 which also implements user ownership on notes. Author: Hayssam Saleh <hayssam.saleh@ebiznext.com> Closes #586 from hayssams/shiro-security-v2 and squashes the following commits: 47421b8 [Hayssam Saleh] Rollback classpath change since zeppelin conf dir already in classpath 5485dcd [Hayssam Saleh] Updates licences for shiro-core and shiro-web introduced in this PR 7200e77 [Hayssam Saleh] Default ticket / principal to anonymous in websocket message 30736a0 [Hayssam Saleh] Add support for cross site requests with credentials 1372231 [Hayssam Saleh] Test mode requires to user baseUrlSrv to connect to the REST API 96ec240 [Hayssam Saleh] use standard HTML tags for SECURITY-README.md 01ba543 [Hayssam Saleh] get ticket before Angular is bootstrapped 2a9e275 [Hayssam Saleh] Add implementation notes 96d1fac [Hayssam Saleh] correct comment in SECURITY-README and keep anonymous policy by default in zeppelin-site.xml.template 6fd9982 [Hayssam Saleh] Add minimal shiro.ini file for test phase 8eee51d [Hayssam Saleh] Remove cache optimization in shiro since it references stormpath and comes from there. 2017925 [Hayssam Saleh] exclude SECURITY-README from rat check f9b1952 [Hayssam Saleh] The Websocket channel is now as secure as the HTTP channel. e2affca [Hayssam Saleh] Securing the HTTP channel only. Websocket security is done in the next commit
Hi Hayssams , the authentication letter is applied but in terms of exception as i am not getting any window for putting my username and password . I am in the customer network so it is not asking for any ausername and password . Showing the message ERROR [2016-06-02 14:27:22,265]({qtp513169028-34} NotebookServer.java[onMessage]:179) - Can't handle message |
### What is this PR for? [SECURITY-README.md](https://github.com/apache/zeppelin/blob/master/SECURITY-README.md) was added by #53 when Shiro auth was implemented for the first time. But I think we need to keep "Security Setup" information in one source; [Official docs website](https://zeppelin.apache.org/docs/0.7.0-SNAPSHOT/security/shiroauthentication.html#security-setup) and guide ppl to see this official docs page so that `SECURITY-README.md` can only contain dev related contents (not step by step setup guide). ### What type of PR is it? Documentation ### What is the Jira issue? N/A ### How should this be tested? Just clicking "View" would be enough I guess :) ### Questions: * Does the licenses files need update? N/A * Is there breaking changes for older versions? N/A * Does this needs documentation? N/A Author: AhyoungRyu <fbdkdud93@hanmail.net> Closes #1829 from AhyoungRyu/remove/duplicatedSection and squashes the following commits: d4b80b8 [AhyoungRyu] Minor update 8ce33be [AhyoungRyu] Remove note sentence 3e867ac [AhyoungRyu] Fix wired numbering d908da9 [AhyoungRyu] Remove outdated 'Security Setup' section in SECURITY-README
Added shiro security. HTTP calls and websockets communications are both protected.