Skip to content
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-1144]Zeppelin home page should only list notebooks with read or write permission #1330

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@mwkang
Copy link
Contributor

commented Aug 13, 2016

What is this PR for?

If logged in user does not have Read and Write permission for a notebook, user should not see the notebook in the zeppelin home page.

What type of PR is it?

Bug Fix

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1144

How should this be tested?

  • unit test
  • online test

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no
@mwkang

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2016

CI error

Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 76.102 sec - in org.apache.zeppelin.integration.SparkParagraphIT

Results :

Tests in error: 
  ZeppelinIT.testAngularDisplay:118->AbstractZeppelinIT.waitForParagraph:70->AbstractZeppelinIT.pollingWait:96 쨩 Timeout
  AuthenticationIT.testGroupPermission:179->AbstractZeppelinIT.pollingWait:96 쨩 Timeout

Tests run: 15, Failures: 0, Errors: 2, Skipped: 0
@bzz

This comment has been minimized.

Copy link
Member

commented Aug 16, 2016

Thank you for fixing it @mwkang

CI failure looks somehow relevant, you can not reproduce it locally?

@mwkang

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2016

@bzz I will check it. Thanks for review.
When I fix CI, I'll mention you.

@mwkang mwkang changed the title [ZEPPELIN-1144]Zeppelin home page should only list notebooks with read or write permission [WIP][ZEPPELIN-1144]Zeppelin home page should only list notebooks with read or write permission Aug 20, 2016

@mwkang mwkang force-pushed the mwkang:ZEPPELIN-1144 branch from 0a011db to 82beb18 Aug 20, 2016

@mwkang mwkang changed the title [WIP][ZEPPELIN-1144]Zeppelin home page should only list notebooks with read or write permission [ZEPPELIN-1144]Zeppelin home page should only list notebooks with read or write permission Aug 20, 2016

@mwkang

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2016

@bzz #1330 (comment) It did not reproduce in local. Can you check this?

@bzz

This comment has been minimized.

Copy link
Member

commented Sep 2, 2016

@mwkang could you please try close and then re-open this PR to trigger the CI and see if the same issue persists? Thanks!

\cc @khalidhuseynov for review.

@khalidhuseynov

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2016

@mwkang thanks for the contribution! actually this is related to multi-user note management and there's some work going on under #1392 or #1390 .

In your case, i tested it, and there's a bit tricky moment with notes reloading and broadcasting. For example if you login as user1 with noteA and user2 with noteB and initially everything looks as expected, and then you reload your notes as shown below with refresh icon:
screen shot 2016-09-02 at 5 52 10 pm
then if you refresh for user1 then noteA will appear in the list of all users, including user2 in our case. Same would happen if you reload for user2, meaning noteB will appear in the list of user1.

@echarles

This comment has been minimized.

Copy link
Member

commented Sep 5, 2016

If the current single notebook-authorization.json is managed by user (with multiple files, and no more with a single one), how would you handle it?

@khalidhuseynov

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

@mwkang could you re-trigger CI?

@khalidhuseynov

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

I think it doesn't solve all problems, but definitely in the roadmap. LGTM

@bzz

This comment has been minimized.

Copy link
Member

commented Sep 13, 2016

Thank you @mwkang @khalidhuseynov !

Will merge to master, if there is no further discussion.

@Leemoonsoo

This comment has been minimized.

Copy link
Member

commented Sep 13, 2016

@mwkang @khalidhuseynov Could you handle or at least create issues for expected problems after merge this PR? I can at least think following two issues,

  • Note list broadcasting message is not aware user session, so notebook list can display notes that supposed to not see when other user refresh the notebook list.
  • When a user changes the permission of a note, note list need to be broadcasted immediately so all connected users can have updated list of notebook. Otherwise, there's still some chances that a user able to see some note on notebook list that they can not access.
@khalidhuseynov

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2016

@Leemoonsoo issues are created under ZEPPELIN-1437 and ZEPPELIN-1438 correspondingly

@Leemoonsoo

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

Thanks, Merge to master if there're no further discussions.

@asfgit asfgit closed this in 2902189 Sep 16, 2016

asfgit pushed a commit that referenced this pull request Sep 17, 2016

[HOT FIX][ZEPPELIN-1144] Fix compilation errors in Notebook.java
### What is this PR for?
After #1330 merged, the latest master build failed with below compilation errors.

```
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /Users/ahyoungryu/Dev/zeppelin/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java:[553,31] cannot find symbol
  symbol:   method id()
  location: variable note1 of type org.apache.zeppelin.notebook.Note
[ERROR] /Users/ahyoungryu/Dev/zeppelin/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java:[557,31] cannot find symbol
  symbol:   method id()
  location: variable note2 of type org.apache.zeppelin.notebook.Note
```

### What type of PR is it?
 Hot Fix

### What is the Jira issue?

### How should this be tested?
 - Build the latest master branch with `mvn clean package -DskipTests` -> compilation error in `zeppelin-zengine`

 - Apply this patch and build with `mvn clean package -DskipTests` -> build success
You can also check #1330 works properly.

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: AhyoungRyu <fbdkdud93@hanmail.net>

Closes #1432 from AhyoungRyu/hotfix/ZEPPELIN-1144 and squashes the following commits:

6a3dbd3 [AhyoungRyu] Fix build error in Notebook.java
@mwkang

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2016

I'm sorry for no-reply long time. my plate was full.
I will finish other 2 PRs and then If ZEPPELIN-1437 and ZEPPELIN-1438 are status is unresolved, I will
try to resolve.
If it is okay..

Thanks for kindly review.

@prabhjyotsingh prabhjyotsingh referenced this pull request Sep 20, 2016

Closed

ZEPPELIN-1456: Flaky Test: AuthenticationIT #1444

0 of 1 task complete

asfgit pushed a commit that referenced this pull request Sep 21, 2016

ZEPPELIN-1456: Flaky Test: AuthenticationIT
### What is this PR for?
This started happening after ZEPPELIN-1144, #1330.
This test (testGroupPermission) is to validate the group related permission on a notebook.

### What type of PR is it?
[Bug Fix]

### Todos
* [ ] - Fix CI

### What is the Jira issue?
* [ZEPPELIN-1456](https://issues.apache.org/jira/browse/ZEPPELIN-1456)

### How should this be tested?
CI xxx.10 (selenium) should be green

### Screenshots (if appropriate)

### 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: Prabhjyot Singh <prabhjyotsingh@gmail.com>

Closes #1444 from prabhjyotsingh/ZEPPELIN-1456 and squashes the following commits:

3c81587 [Prabhjyot Singh] ZEPPELIN-1456: fix

asfgit pushed a commit that referenced this pull request Oct 18, 2016

[ZEPPELIN-1437, 1438] Multi-user note management - user aware reload …
…broadcast

### What is this PR for?
This PR addresses part of multi-user note management in Zeppelin. One of the tasks namely listing notes per user on Zeppelin start was addressed in #1330. However that PR didn't solve all problems, and reloading notes was incomplete as well as socket broadcast was not user aware [ZEPPELIN-1437](https://issues.apache.org/jira/browse/ZEPPELIN-1437), [ZEPPELIN-1438](https://issues.apache.org/jira/browse/ZEPPELIN-1438). This PR addresses those issue.

### What type of PR is it?
Improvement

### Todos
* [x] - list notes per user on reload
* [x] - broadcast per user (multicast)
* [x] - tests
* [x] - use authorization module to filter notes on sync
* [x] - broadcast on permissions change
* [ ] - discussion and review

### What is the Jira issue?
[Zeppelin-1437](https://issues.apache.org/jira/browse/ZEPPELIN-1437), [ZEPPELIN-1438](https://issues.apache.org/jira/browse/ZEPPELIN-1438)

### How should this be tested?
1. Start Zeppelin
2. Login as user1, and user2 on different windows
3. Each user should be able to see their own note workbench
4. If note changed to private (readers, writers not empty), that note should disappear from others note workbench.

### Screenshots (if appropriate)
![reload_broadcast](https://cloud.githubusercontent.com/assets/1642088/18679507/e4a0161c-7f9a-11e6-9d57-0930abf4b780.gif)

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? yes

Author: Khalid Huseynov <khalidhnv@gmail.com>

Closes #1392 from khalidhuseynov/feat/multi-user-notes and squashes the following commits:

a2ce268 [Khalid Huseynov] broadcast note list on perm update - zeppelin-1438
9cf1d88 [Khalid Huseynov] fix init not to initialize every time
17eae84 [Khalid Huseynov] bugfix: add precondition for NP
781207e [Khalid Huseynov] bugfix: reload only once
537cc0e [Khalid Huseynov] apply filter from authorization in sync
09e6723 [Khalid Huseynov] notebookAuthorization as singleton
9427e62 [Khalid Huseynov] multicast fine grained note lists to users instead of broadcast
6614e2b [Khalid Huseynov] improve tests
1399407 [Khalid Huseynov] remove unused imports
d9c3bc9 [Khalid Huseynov] filter reload using predicates
92f37f5 [Khalid Huseynov] substitute old getAllNotes(subject) with new implementation
b7f19c9 [Khalid Huseynov] separate getAllNotes() and getAllNotes(subject)
17e2d4c [Khalid Huseynov] first draft

darionyaphet added a commit to darionyaphet/zeppelin that referenced this pull request Oct 27, 2016

[ZEPPELIN-1437, 1438] Multi-user note management - user aware reload …
…broadcast

### What is this PR for?
This PR addresses part of multi-user note management in Zeppelin. One of the tasks namely listing notes per user on Zeppelin start was addressed in apache#1330. However that PR didn't solve all problems, and reloading notes was incomplete as well as socket broadcast was not user aware [ZEPPELIN-1437](https://issues.apache.org/jira/browse/ZEPPELIN-1437), [ZEPPELIN-1438](https://issues.apache.org/jira/browse/ZEPPELIN-1438). This PR addresses those issue.

### What type of PR is it?
Improvement

### Todos
* [x] - list notes per user on reload
* [x] - broadcast per user (multicast)
* [x] - tests
* [x] - use authorization module to filter notes on sync
* [x] - broadcast on permissions change
* [ ] - discussion and review

### What is the Jira issue?
[Zeppelin-1437](https://issues.apache.org/jira/browse/ZEPPELIN-1437), [ZEPPELIN-1438](https://issues.apache.org/jira/browse/ZEPPELIN-1438)

### How should this be tested?
1. Start Zeppelin
2. Login as user1, and user2 on different windows
3. Each user should be able to see their own note workbench
4. If note changed to private (readers, writers not empty), that note should disappear from others note workbench.

### Screenshots (if appropriate)
![reload_broadcast](https://cloud.githubusercontent.com/assets/1642088/18679507/e4a0161c-7f9a-11e6-9d57-0930abf4b780.gif)

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? yes

Author: Khalid Huseynov <khalidhnv@gmail.com>

Closes apache#1392 from khalidhuseynov/feat/multi-user-notes and squashes the following commits:

a2ce268 [Khalid Huseynov] broadcast note list on perm update - zeppelin-1438
9cf1d88 [Khalid Huseynov] fix init not to initialize every time
17eae84 [Khalid Huseynov] bugfix: add precondition for NP
781207e [Khalid Huseynov] bugfix: reload only once
537cc0e [Khalid Huseynov] apply filter from authorization in sync
09e6723 [Khalid Huseynov] notebookAuthorization as singleton
9427e62 [Khalid Huseynov] multicast fine grained note lists to users instead of broadcast
6614e2b [Khalid Huseynov] improve tests
1399407 [Khalid Huseynov] remove unused imports
d9c3bc9 [Khalid Huseynov] filter reload using predicates
92f37f5 [Khalid Huseynov] substitute old getAllNotes(subject) with new implementation
b7f19c9 [Khalid Huseynov] separate getAllNotes() and getAllNotes(subject)
17e2d4c [Khalid Huseynov] first draft

pedrozatta added a commit to pedrozatta/zeppelin that referenced this pull request Oct 27, 2016

[ZEPPELIN-1144]Zeppelin home page should only list notebooks with rea…
…d or write permission

### What is this PR for?
If logged in user does not have Read and Write permission for a notebook, user should not see the notebook in the zeppelin home page.

### What type of PR is it?
Bug Fix

### Todos

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1144

### How should this be tested?
* unit test
* online test

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Minwoo Kang <minwoo.kang@outlook.com>

Closes apache#1330 from mwkang/ZEPPELIN-1144 and squashes the following commits:

82beb18 [Minwoo Kang] User see read, write, owner permission notebook.

pedrozatta added a commit to pedrozatta/zeppelin that referenced this pull request Oct 27, 2016

[HOT FIX][ZEPPELIN-1144] Fix compilation errors in Notebook.java
### What is this PR for?
After apache#1330 merged, the latest master build failed with below compilation errors.

```
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /Users/ahyoungryu/Dev/zeppelin/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java:[553,31] cannot find symbol
  symbol:   method id()
  location: variable note1 of type org.apache.zeppelin.notebook.Note
[ERROR] /Users/ahyoungryu/Dev/zeppelin/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java:[557,31] cannot find symbol
  symbol:   method id()
  location: variable note2 of type org.apache.zeppelin.notebook.Note
```

### What type of PR is it?
 Hot Fix

### What is the Jira issue?

### How should this be tested?
 - Build the latest master branch with `mvn clean package -DskipTests` -> compilation error in `zeppelin-zengine`

 - Apply this patch and build with `mvn clean package -DskipTests` -> build success
You can also check apache#1330 works properly.

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: AhyoungRyu <fbdkdud93@hanmail.net>

Closes apache#1432 from AhyoungRyu/hotfix/ZEPPELIN-1144 and squashes the following commits:

6a3dbd3 [AhyoungRyu] Fix build error in Notebook.java

pedrozatta added a commit to pedrozatta/zeppelin that referenced this pull request Oct 27, 2016

ZEPPELIN-1456: Flaky Test: AuthenticationIT
### What is this PR for?
This started happening after ZEPPELIN-1144, apache#1330.
This test (testGroupPermission) is to validate the group related permission on a notebook.

### What type of PR is it?
[Bug Fix]

### Todos
* [ ] - Fix CI

### What is the Jira issue?
* [ZEPPELIN-1456](https://issues.apache.org/jira/browse/ZEPPELIN-1456)

### How should this be tested?
CI xxx.10 (selenium) should be green

### Screenshots (if appropriate)

### 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: Prabhjyot Singh <prabhjyotsingh@gmail.com>

Closes apache#1444 from prabhjyotsingh/ZEPPELIN-1456 and squashes the following commits:

3c81587 [Prabhjyot Singh] ZEPPELIN-1456: fix

pedrozatta added a commit to pedrozatta/zeppelin that referenced this pull request Oct 27, 2016

[ZEPPELIN-1437, 1438] Multi-user note management - user aware reload …
…broadcast

### What is this PR for?
This PR addresses part of multi-user note management in Zeppelin. One of the tasks namely listing notes per user on Zeppelin start was addressed in apache#1330. However that PR didn't solve all problems, and reloading notes was incomplete as well as socket broadcast was not user aware [ZEPPELIN-1437](https://issues.apache.org/jira/browse/ZEPPELIN-1437), [ZEPPELIN-1438](https://issues.apache.org/jira/browse/ZEPPELIN-1438). This PR addresses those issue.

### What type of PR is it?
Improvement

### Todos
* [x] - list notes per user on reload
* [x] - broadcast per user (multicast)
* [x] - tests
* [x] - use authorization module to filter notes on sync
* [x] - broadcast on permissions change
* [ ] - discussion and review

### What is the Jira issue?
[Zeppelin-1437](https://issues.apache.org/jira/browse/ZEPPELIN-1437), [ZEPPELIN-1438](https://issues.apache.org/jira/browse/ZEPPELIN-1438)

### How should this be tested?
1. Start Zeppelin
2. Login as user1, and user2 on different windows
3. Each user should be able to see their own note workbench
4. If note changed to private (readers, writers not empty), that note should disappear from others note workbench.

### Screenshots (if appropriate)
![reload_broadcast](https://cloud.githubusercontent.com/assets/1642088/18679507/e4a0161c-7f9a-11e6-9d57-0930abf4b780.gif)

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? yes

Author: Khalid Huseynov <khalidhnv@gmail.com>

Closes apache#1392 from khalidhuseynov/feat/multi-user-notes and squashes the following commits:

a2ce268 [Khalid Huseynov] broadcast note list on perm update - zeppelin-1438
9cf1d88 [Khalid Huseynov] fix init not to initialize every time
17eae84 [Khalid Huseynov] bugfix: add precondition for NP
781207e [Khalid Huseynov] bugfix: reload only once
537cc0e [Khalid Huseynov] apply filter from authorization in sync
09e6723 [Khalid Huseynov] notebookAuthorization as singleton
9427e62 [Khalid Huseynov] multicast fine grained note lists to users instead of broadcast
6614e2b [Khalid Huseynov] improve tests
1399407 [Khalid Huseynov] remove unused imports
d9c3bc9 [Khalid Huseynov] filter reload using predicates
92f37f5 [Khalid Huseynov] substitute old getAllNotes(subject) with new implementation
b7f19c9 [Khalid Huseynov] separate getAllNotes() and getAllNotes(subject)
17e2d4c [Khalid Huseynov] first draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.