[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[weand]

Github user weand commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
great. LGTM :+1: please merge to master and branch-0.8


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[zjffdu]

Github user zjffdu commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
Thanks @weand for review, comments are addressed


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[weand]

Github user weand commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@zjffdu uhm, I can't confirm it. Today after checking out this PR from 
scratch my first point now works correctly. sorry for the spam.

created issue 
[ZEPPELIN-3427](https://issues.apache.org/jira/browse/ZEPPELIN-3427) for the 
second point. is that really so complex to wait for 0.8.1 or 0.9.0 ?

Part of that change would be:
1) moving/adding logic of setting note.config.cronExecutionUser + 
cronExecutionRoles to 
org.apache.zeppelin.socket.NotebookServer.isCronUpdated(Map, 
Map) when _cronUpdated = true_.
2) Then evaluate those config properties in 
org.apache.zeppelin.notebook.Note.runAll() when authenticationInfo is created




---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[zjffdu]

Github user zjffdu commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@weand I don't the first issue you mentioned, can you confirm that ? 
For the second point, I am afraid this is not trivial to fix, I would 
suggest to defer it to 0.8.1 or 0.9, what do you think ? 


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[mebelousov]

Github user mebelousov commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@weand Thank you!
Some addition to 2.
Over time user may not belong to group.
At first we could store cronExecutingRoles and in future it's to be good to 
check groups on the fly.


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[weand]

Github user weand commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
The notebook now runs successfully after restart with the user who enabled 
the scheduler.

2 remaining issues:

1) user1 schedules the cron. then the note is executed via cron with 
'user1' only as long as nobody else runs the note. as soon as 'user2' runs the 
note manually, the cron will also use 'user2' from that time on. Endusers would 
expect the cron will always run as 'user1'.

2) One more valid but yet uncovered use case left in my mind (dealing with 
permissions):
- interpreter permissions have been set only for groups (e.g. DepartmentA 
should be allowed to run spark interpreter)
- user created note with %spark interpreter and schedules a cron
- now when cron runs, **only username is added to authentication info. 
groups of the user (e.g. DepartmentA in that example) are omitted** and the 
notebook fails with an error, that **user 'xyz' has no permission for running 
the interpreter**.

any chance to fix that case as well? 
I think of something like introducing cronExecutingRoles (in addition to  
cronExecutingUser) and adding all roles of the user who enables the cron to 
that config property. And then when a cron runs, all that roles are added to 
the corresponding authentication info.


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[weand]

Github user weand commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
The notebook now runs successfully after restart with the user who enabled 
the scheduler.

There is one more valid use case left in my mind (dealing with permissions):
- interpreter permissions have been set only for groups (e.g. DepartmentA 
should be allowed to run spark interpreter)
- user created note with %spark interpreter and schedules a cron
- now when cron runs, only username is added to authentication info. groups 
of the user (e.g. DepartmentA in that example) are omitted and the notebook 
fails with an error, that user 'xyz' has no permission for running spark 
interpreter.

any chance to fix that case as well? 
I think of something like introducing cronExecutingRoles (in addition to  
cronExecutingUser) and adding all roles of the user who enables the cron to 
that config property. And then when a cron runs, all that roles are added to 
the corresponding authentication info.


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[zjffdu]

Github user zjffdu commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
Thanks @weand , I agree with you that your proposal seems much easier. I 
have updated the PR, please help verify it, thanks 


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[weand]

Github user weand commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@mebelousov I fully agree that running document as the group is not what a 
user would expect.

That's why I'm proposing another approach again:
1) prior to #2914 the cronExecutionUser config property was added to 
org.apache.zeppelin.notebook.Note.config when activating the cron and entering 
a username
2) that property is not set anymore. this PR here only sets the 
authentication info at running time to something (a principal) from the 
permission tab (which can be user or groups, which can not be determined 
properly)
3) so why not again set that cronExecutionUser config property from 1) 
(under the hood): with the user who activates the cron schedule expression. 
when only the owner can enable cron expressions, it will always be the username 
regardless of what principals exactly are configured in the permissions tab.


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[zjffdu]

Github user zjffdu commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
I am saying using group is not good, it just depends on user's setting. 
BTW, it looks like this PR also works for your scenario. In this PR, I would 
always choose the owner to run the cronjob. And in your case, you just set the 
owner as group. Do I understand it correctly ? :)




---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[mebelousov]

Github user mebelousov commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@zjffdu This is your solution ;) I have tested your branch.

Do we have uniform opinion that running document as the group is not good?


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[zjffdu]

Github user zjffdu commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@mebelousov Your solution depends on shiro setting, we  could not assume 
that in code.


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[mebelousov]

Github user mebelousov commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@weand cronExecutionUser is set to the user who set the cron string inside 
the note.
@zjffdu I define groups in shiro.ini.
The user sets groupname in owner field. Then the note runs on cron from 
this groupname.


![image](https://user-images.githubusercontent.com/9324163/38917720-3fa94b32-42f4-11e8-938d-d68ac459d511.png)



---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[zjffdu]

Github user zjffdu commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@mebelousov What do you mean `group as owner` ? And how do you use that ?




---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[weand]

Github user weand commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
and optionally (or alternatively) introduce a config parameter for a static 
cronExecutionUser (kind of service user) to be defined by ops team. when this 
config parameter is set, cron schedules will always use that user.


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[weand]

Github user weand commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@zjffdu is there a chance to set the cronExecutionUser implictly to the 
user who activates the cron schedule. 
and in addtion only allow owners (which can be defined by username or 
groupnames) to activate the cron schedule.


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[mebelousov]

Github user mebelousov commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@zjffdu I have checked with group as owner. This is ok.
Why do you think that use of owner is more secure than cronExecutingUser?


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[zjffdu]

Github user zjffdu commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@mebelousov Currently I would only allow note owner to set the schedule and 
run cron job via the owner. But seems zeppelin allow multiple owners for one 
note (it doesn't make sense to me), this is only what I can do the eliminate 
the security issue. 


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[mebelousov]

Github user mebelousov commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
Groups can be note owners. As I see `AuthenticationInfo` is not valid for 
internal shiro groups.

How can I find the user which scheduled note?


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[zjffdu]

Github user zjffdu commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
Update the PR to run the cron job as the note owner 


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[zjffdu]

Github user zjffdu commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
Thanks @weand , even I didn't realize the user in note.json means the last 
user run the paragraph (I thought it is the owner). In that case, it seems 
meaningless to store user in note.json IMHO.  
Regarding the issue, I will update it soon


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[weand]

Github user weand commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
so running via cron will use the last user that executed a paragraph. that 
does not fit exactly to the docu change from original PR:
> Cron executing user (It is removed from 0.8 where it **enforces the cron 
execution user to be the note owner** for security purpose)

[https://github.com/apache/zeppelin/pull/2914/files#diff-bed1be4fb1f7f6c8e3bd6db18d72dc44](https://github.com/apache/zeppelin/pull/2914/files#diff-bed1be4fb1f7f6c8e3bd6db18d72dc44)


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[r-kamath]

Github user r-kamath commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
LGTM


---

[GitHub] zeppelin issue #2925: ZEPPELIN-3404. Fail to run cronjob when user doesn't r...

[zjffdu]

Github user zjffdu commented on the issue:

https://github.com/apache/zeppelin/pull/2925
  
@felixcheung Mind to help review it ?


---