[ZEPPELIN-2647] Bypass auth logic when a user logins as admin role #2585

yu74n · 2017-09-13T12:52:27Z

What is this PR for?

For administrator, make new admin role that assigned user can see all notebooks.

What type of PR is it?

Improvement

What is the Jira issue?

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

How should this be tested?

Set role name to use as admin through ZEPPELIN_OWNER_ROLE = or zeppelin.owner.role = .
Default role name is admin
Login as user who is not assigned as admin and create notebook.
Logout the user and login another user who is assigned as admin, open the created notebook.

Questions:

Does the licenses files need update? N
Is there breaking changes for older versions? Y/N
Does this needs documentation? Y

yu74n · 2017-09-13T12:53:06Z

continued from #2525 #2416

1ambda · 2017-09-15T06:37:41Z

Thanks for the contribution @yu74n
Looks Good to Me. Let me test more cases and then left feedback soon.

felixcheung · 2017-09-23T07:36:07Z

conf/zeppelin-site.xml.template

@@ -384,6 +384,12 @@
 </property>

 <property>
+  <name>zeppelin.owner.role</name>


I feel the property name might not be very intuitive, how about calling it zeppelin.notebook.default.owner.username or something similar?

Thank you for reviewing. It would be better, I think too. I'll fix it.

1ambda · 2017-10-02T05:32:31Z

LGTM.

1ambda · 2017-10-11T10:42:31Z

I wanted to merge now. But cound't find the build history.

https://builds.apache.org/job/zeppelin-pull-request/2960/

@yu74n if you don't mind close and reopen this issue to trigger jenkins?

yu74n · 2017-10-11T11:21:08Z

@1ambda Sure. I've done that

1ambda · 2017-10-11T11:43:07Z

The 3/4 jobs failed. Please rebase this PR based on master. And Could you resolve them?

yu74n · 2017-10-12T03:40:25Z

I rebased and ran test again, but this test is still failed.
https://travis-ci.org/yu74n/zeppelin/jobs/286508544

I think probably my PR doesn't affect the test.
I ran test several times, but failed tests were changed each time.

1ambda · 2017-10-12T05:16:17Z

Hi, Thanks for rebasing and testing. But this recent PR passed the CI.
If you don't mind please re-run few more times and check the failure reasons for every trial?

If we can verify it's a flaky test, we can merge, but without evidence, we can't do that.

felixcheung

not sure this is related to this PR but not clear how this test can fail

  InterpreterModeActionsIT.testPerUserScopedAction:564 The number of python process is
Expected: "2"
     but: was "1"

yu74n · 2017-10-18T09:39:14Z

Rerun all tests.
'testPerUserScopedAction' is passed this time, but other 4 tests are failed.
These 4 tests were passed before.

ZeppelinSparkClusterTest.pySparkDepLoaderTest:478 expected:<FINISHED> but was:<ERROR>
ZeppelinSparkClusterTest.pySparkAutoConvertOptionTest:358 expected:<FINISHED> but was:<ERROR>
ZeppelinSparkClusterTest.pySparkTest:259 expected:<FINISHED> but was:<ERROR>
ZeppelinSparkClusterTest.sparkRTest:209 expected:<[[1] 3]> but was:<[<pre><code>Error in invokeJava(isStatic = TRUE, className, methodName, ...): java.lang.IllegalStateException: Cannot call methods on a stopped SparkContext.

yu74n · 2017-11-02T02:08:28Z

All tests are passed, but NotebookTest has an error.
java.lang.RuntimeException: org.apache.thrift.transport.TTransportException: java.net.SocketException: Connection reset

yu74n · 2017-11-02T08:57:44Z

and rerun the failed travis job manually, the job is passed.
https://travis-ci.org/yu74n/zeppelin/jobs/295676369

yu74n · 2017-11-02T08:59:16Z

What do you think about that? @1ambda @felixcheung

felixcheung

btw, what if zeppelin.notebook.default.owner.username is unset or set to ""?

yu74n · 2017-11-06T06:55:07Z

@felixcheung if unset zeppelin.notebook.default.owner.username, set 'admin' as default value.
https://github.com/apache/zeppelin/pull/2585/files#diff-179ea3a816210bb9904baabec1a552e4R716

felixcheung

hmm, ok if it has

  <name>zeppelin.notebook.default.owner.username</name>
   <value></value>

?

yu74n · 2017-11-06T10:57:52Z

@felixcheung
I've changed default value is blank.
if set blank as zeppelin.notebook.default.owner.username, disable admin role.

felixcheung · 2017-11-07T07:16:08Z

ok, could you explain how

if set blank as zeppelin.notebook.default.owner.username, disable admin role.

?
I think also it'll be good to match template to the default value

yu74n · 2017-11-07T15:13:08Z

if set blank as default owner username, apply blank('') to the admin role. but, I think, a user cannot set blank as login user name. so nearly equal as disable admin role.
if a user would create blank user or blank role, the user can access all notebooks though.

felixcheung · 2017-11-07T17:47:27Z

hmm, since this is around security perhaps we need to be more careful?
how about we add a check for the owner username is valid (at least not "") and not add to the admin role if it is not valid?

yu74n · 2017-11-09T06:39:58Z

Actually, I thought so too when I explained how if the owner name is blank. I've fixed that.
I've changed config template to be disable admin role.
Could you review that again?
@felixcheung

felixcheung · 2017-11-09T07:35:36Z

conf/zeppelin-site.xml.template

@@ -394,6 +394,12 @@
 </property>

 <property>
+  <name>zeppelin.notebook.default.owner.username</name>
+  <value></value>
+  <description>Set owner role by default in private mode</description>


btw, did we say this isn't just for private mode any more?

oops, You are right. This is for both public and private. I've fixed it.

felixcheung · 2017-11-09T07:51:07Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NotebookAuthorization.java

+
+  private boolean isAdmin(Set<String> entities) {
+    String adminRole = conf.getString(ConfVars.ZEPPELIN_OWNER_ROLE);
+    if (adminRole.isEmpty()) {


please use org.apache.commons.lang.StringUtils IsBlank()

Thanks, fixed it.

felixcheung · 2017-11-09T17:54:49Z

could you check if it's related? https://travis-ci.org/yu74n/zeppelin/builds/299517426

felixcheung

LGTM pending tests

yu74n · 2017-11-09T21:00:28Z

CI failed at testRunOnSelectionChange()

java.lang.AssertionError: Even if 'RunOnSelectionChange' is set as false, still can run the paragraph by pressing ENTER 
Expected: "My selection is 1"
but: was "My selection is 2"

This test is related to 'Run on selection change' checkbox in paragraph menu.
I think the reason why that test is failed would be timing error, cause of the test code ran the paragraph before the checkbox was unchecked. Then I think the test is not related to my PR.

I rerun the CI job manually, all tests are passed.
https://travis-ci.org/yu74n/zeppelin/builds/299517426

felixcheung · 2017-11-11T06:58:01Z

ok thanks

felixcheung · 2017-11-11T06:58:09Z

merging if no more comment

For administrator, make new admin role that assigned user can see all notebooks. Improvement https://issues.apache.org/jira/browse/ZEPPELIN-2647 1. Set role name to use as admin through ZEPPELIN_OWNER_ROLE = <role name> or zeppelin.owner.role = <role name>. Default role name is admin 2. Login as user who is not assigned as admin and create notebook. 3. Logout the user and login another user who is assigned as admin, open the created notebook. * Does the licenses files need update? N * Is there breaking changes for older versions? Y/N * Does this needs documentation? Y Author: Yuta Hongo <yutago@gmail.com> Closes apache#2585 from yu74n/bypass-auth-logic and squashes the following commits: c706302 [Yuta Hongo] Use StringUtils isBlank() f6c6345 [Yuta Hongo] Remove description mentioned about private mode c6e1382 [Yuta Hongo] Disable admin role by default 0170b3f [Yuta Hongo] Check if admin role is valid or not 532a49f [Yuta Hongo] Set blank as default.owner.username default value 98a9de0 [Yuta Hongo] Rename property name 26b818c [Yuta Hongo] Make admin role to bypass auth logic

yu74n mentioned this pull request Sep 13, 2017

[ZEPPELIN-2647] Make admin role to bypass auth logic #2525

Closed

yu74n changed the title ~~Bypass auth logic when a user logins as admin role~~ [ZEPPELIN-2647] Bypass auth logic when a user logins as admin role Sep 14, 2017

felixcheung reviewed Sep 23, 2017

View reviewed changes

yu74n force-pushed the bypass-auth-logic branch from 84556a5 to 0dd1d72 Compare September 26, 2017 05:10

yu74n closed this Oct 11, 2017

yu74n reopened this Oct 11, 2017

yu74n force-pushed the bypass-auth-logic branch from 0dd1d72 to cfcea0d Compare October 11, 2017 13:06

yu74n closed this Oct 12, 2017

yu74n reopened this Oct 12, 2017

yu74n closed this Oct 12, 2017

yu74n reopened this Oct 12, 2017

yu74n closed this Oct 12, 2017

yu74n reopened this Oct 12, 2017

yu74n closed this Oct 13, 2017

yu74n reopened this Oct 13, 2017

felixcheung reviewed Oct 13, 2017

View reviewed changes

yu74n force-pushed the bypass-auth-logic branch from cfcea0d to c461608 Compare October 18, 2017 07:47

yu74n added 2 commits November 1, 2017 19:04

Make admin role to bypass auth logic

26b818c

Rename property name

98a9de0

yu74n force-pushed the bypass-auth-logic branch from c461608 to 98a9de0 Compare November 1, 2017 10:06

felixcheung reviewed Nov 3, 2017

View reviewed changes

felixcheung reviewed Nov 6, 2017

View reviewed changes

Set blank as default.owner.username default value

532a49f

yu74n closed this Nov 7, 2017

yu74n reopened this Nov 7, 2017

yu74n added 2 commits November 9, 2017 15:32

Check if admin role is valid or not

0170b3f

Disable admin role by default

c6e1382

felixcheung reviewed Nov 9, 2017

View reviewed changes

yu74n added 2 commits November 9, 2017 17:14

Remove description mentioned about private mode

f6c6345

Use StringUtils isBlank()

c706302

felixcheung approved these changes Nov 9, 2017

View reviewed changes

asfgit closed this in 717a8c1 Nov 15, 2017

yu74n mentioned this pull request Nov 15, 2017

[ZEPPELIN-2647] Set admin user as owner when user create a notebook #2416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZEPPELIN-2647] Bypass auth logic when a user logins as admin role #2585

[ZEPPELIN-2647] Bypass auth logic when a user logins as admin role #2585

yu74n commented Sep 13, 2017

yu74n commented Sep 13, 2017

1ambda commented Sep 15, 2017

felixcheung Sep 23, 2017

yu74n Sep 24, 2017

1ambda commented Oct 2, 2017

1ambda commented Oct 11, 2017

yu74n commented Oct 11, 2017 •

edited

1ambda commented Oct 11, 2017 •

edited

yu74n commented Oct 12, 2017 •

edited

1ambda commented Oct 12, 2017 •

edited

felixcheung left a comment

yu74n commented Oct 18, 2017 •

edited

yu74n commented Nov 2, 2017

yu74n commented Nov 2, 2017

yu74n commented Nov 2, 2017

felixcheung left a comment

yu74n commented Nov 6, 2017

felixcheung left a comment

yu74n commented Nov 6, 2017 •

edited

felixcheung commented Nov 7, 2017 •

edited

yu74n commented Nov 7, 2017 •

edited

felixcheung commented Nov 7, 2017

yu74n commented Nov 9, 2017 •

edited

felixcheung Nov 9, 2017

yu74n Nov 9, 2017 •

edited

felixcheung Nov 9, 2017

yu74n Nov 9, 2017

felixcheung commented Nov 9, 2017

felixcheung left a comment

yu74n commented Nov 9, 2017 •

edited

felixcheung commented Nov 11, 2017

felixcheung commented Nov 11, 2017

[ZEPPELIN-2647] Bypass auth logic when a user logins as admin role #2585

[ZEPPELIN-2647] Bypass auth logic when a user logins as admin role #2585

Conversation

yu74n commented Sep 13, 2017

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

Questions:

yu74n commented Sep 13, 2017

1ambda commented Sep 15, 2017

felixcheung Sep 23, 2017

Choose a reason for hiding this comment

yu74n Sep 24, 2017

Choose a reason for hiding this comment

1ambda commented Oct 2, 2017

1ambda commented Oct 11, 2017

yu74n commented Oct 11, 2017 • edited

1ambda commented Oct 11, 2017 • edited

yu74n commented Oct 12, 2017 • edited

1ambda commented Oct 12, 2017 • edited

felixcheung left a comment

Choose a reason for hiding this comment

yu74n commented Oct 18, 2017 • edited

yu74n commented Nov 2, 2017

yu74n commented Nov 2, 2017

yu74n commented Nov 2, 2017

felixcheung left a comment

Choose a reason for hiding this comment

yu74n commented Nov 6, 2017

felixcheung left a comment

Choose a reason for hiding this comment

yu74n commented Nov 6, 2017 • edited

felixcheung commented Nov 7, 2017 • edited

yu74n commented Nov 7, 2017 • edited

felixcheung commented Nov 7, 2017

yu74n commented Nov 9, 2017 • edited

felixcheung Nov 9, 2017

Choose a reason for hiding this comment

yu74n Nov 9, 2017 • edited

Choose a reason for hiding this comment

felixcheung Nov 9, 2017

Choose a reason for hiding this comment

yu74n Nov 9, 2017

Choose a reason for hiding this comment

felixcheung commented Nov 9, 2017

felixcheung left a comment

Choose a reason for hiding this comment

yu74n commented Nov 9, 2017 • edited

felixcheung commented Nov 11, 2017

felixcheung commented Nov 11, 2017

yu74n commented Oct 11, 2017 •

edited

1ambda commented Oct 11, 2017 •

edited

yu74n commented Oct 12, 2017 •

edited

1ambda commented Oct 12, 2017 •

edited

yu74n commented Oct 18, 2017 •

edited

yu74n commented Nov 6, 2017 •

edited

felixcheung commented Nov 7, 2017 •

edited

yu74n commented Nov 7, 2017 •

edited

yu74n commented Nov 9, 2017 •

edited

yu74n Nov 9, 2017 •

edited

yu74n commented Nov 9, 2017 •

edited