Skip to content

[Zeppelin-945] Interpreter authorization #1257

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.

Sign up for GitHub

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

Closed
astroshim wants to merge 16 commits into from
Closed

[Zeppelin-945] Interpreter authorization #1257

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
407d260
fit UX
Aug 2, 2016
29d8f43
fix js
Aug 3, 2016
b34fdf7
fix for checking intpname
Aug 3, 2016
1ae1c6a
fix for @AhyoungRyu pointed.
Aug 4, 2016
635d523
Merge branch 'master' of https://github.com/apache/zeppelin into ZEPP…
Aug 4, 2016
e56e5b1
wip
Aug 5, 2016
e1679b3
wip
Aug 6, 2016
e72c097
Merge branch 'master' into ZEPPELIN-945_1
Aug 6, 2016
e1e7a35
add interpreter create.
Aug 6, 2016
e42cb9e
fix initialize user value
Aug 6, 2016
febce0c
fix js
Aug 6, 2016
f07542a
remove comment
Aug 9, 2016
3e25159
move directive to componets
Aug 9, 2016
7ed8ad6
fix js code style
Aug 9, 2016
2c48ded
code refactor.
Aug 15, 2016
83097ab
fix naming.
Aug 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions zeppelin-web/src/app/interpreter/interpreter-create/interpreter-create.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,35 @@ <h4>Create new interpreter</h4>
pu-elastic-input-minwidth="180px" ng-model="newInterpreterSetting.option.port" />
</div>

<div class="col-md-12">
<div class="checkbox">
<span class="input-group" style="line-height:30px;">
<label><input type="checkbox" style="width:18px !important" id="idShowPermission" ng-click="togglePermissions('newInterpreter')" ng-model="newInterpreterSetting.option.setPermission"/>
Set permission </label>
</span>
</div>
</div>

<div class="col-md-12">
<!-- permissions -->
<div ng-show="newInterpreterSetting.option.setPermission" class="permissionsForm">
<div>
<p>
Enter comma separated users in the fields. <br />
Empty field (*) implies anyone can run this interpreter.
</p>
<div>

<span class="owners">Owners </span>
<select id="newInterpreterUsers" class="form-control" multiple="multiple">
<option ng-repeat="user in newInterpreterSetting.option.users" selected="selected">{{user}}</option>
</select>
</div>
</div>
</div>
</div>


<b>Properties</b>
<table class="table table-striped properties">
<tr>
Expand Down
92 changes: 89 additions & 3 deletions zeppelin-web/src/app/interpreter/interpreter.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,21 @@
*/
'use strict';

angular.module('zeppelinWebApp').controller('InterpreterCtrl',
angular.module('zeppelinWebApp')
.directive('interpreterDirective', function($timeout) {

@prabhjyotsingh prabhjyotsingh Aug 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move this directive to a different file zeppelin-web/src/components/*

@astroshim astroshim Aug 9, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the directive. Thanks!

return {
restrict: 'A',
link: function(scope, element, attr) {
if (scope.$last === true) {
$timeout(function() {
var id = 'ngRenderFinished';
scope.$emit(id);
});
}
}
};
})
.controller('InterpreterCtrl',
function($scope, $http, baseUrlSrv, ngToast, $timeout, $route) {
var interpreterSettingsTmp = [];
$scope.interpreterSettings = [];
Expand All @@ -22,8 +36,69 @@ angular.module('zeppelinWebApp').controller('InterpreterCtrl',
$scope.showRepositoryInfo = false;
$scope._ = _;

$scope.openPermissions = function() {
$scope.showInterpreterAuth = true;
};

$scope.closePermissions = function() {
$scope.showInterpreterAuth = false;
};

var getSelectJson = function() {
var selectJson = {
tags: false,
multiple: true,
tokenSeparators: [',', ' '],
minimumInputLength: 2,
ajax: {
url: function(params) {
if (!params.term) {
return false;
}
return baseUrlSrv.getRestApiBase() + '/security/userlist/' + params.term;
},
delay: 250,
processResults: function(data, params) {
var users = [];
if (data.body.users.length !== 0) {
for (var i = 0; i < data.body.users.length; i++) {
users.push({
'id': data.body.users[i],
'text': data.body.users[i]
});
}
}
return {
results: users,
pagination: {
more: false
}
};
},
cache: false
}
};
return selectJson;
};

$scope.togglePermissions = function(intpName) {
angular.element('#' + intpName + 'Users').select2(getSelectJson());
if ($scope.showInterpreterAuth) {
$scope.closePermissions();
} else {
$scope.openPermissions();
}
};

$scope.$on('ngRenderFinished', function(event, data) {
for (var setting = 0; setting < $scope.interpreterSettings.length; setting++) {
angular.element('#' + $scope.interpreterSettings[setting].name + 'Users').select2(getSelectJson());
}
});

var getInterpreterSettings = function() {
$http.get(baseUrlSrv.getRestApiBase() + '/interpreter/setting').success(function(data, status, headers, config) {
$http.get(baseUrlSrv.getRestApiBase() + '/interpreter/setting')
.success(function(data, status, headers, config) {
$scope.interpreterSettings = data.body;
checkDownloadingDependencies();
}).error(function(data, status, headers, config) {
Expand Down Expand Up @@ -114,7 +189,6 @@ angular.module('zeppelinWebApp').controller('InterpreterCtrl',
var setting = $scope.interpreterSettings[index];
option = setting.option;
}

if (option.perNoteSession) {
return 'scoped';
} else if (option.perNoteProcess) {
Expand Down Expand Up @@ -148,10 +222,15 @@ angular.module('zeppelinWebApp').controller('InterpreterCtrl',
if (setting.option.isExistingProcess === undefined) {
setting.option.isExistingProcess = false;
}
if (setting.option.setPermission === undefined) {
setting.option.setPermission = false;
}
if (setting.option.remote === undefined) {
// remote always true for now
setting.option.remote = true;
}
setting.option.users = angular.element('#' + setting.name + 'Users').val();

var request = {
option: angular.copy(setting.option),
properties: angular.copy(setting.properties),
Expand All @@ -168,6 +247,7 @@ angular.module('zeppelinWebApp').controller('InterpreterCtrl',
removeTMPSettings(index);
thisConfirm.close();
checkDownloadingDependencies();
$route.reload();
})
.error(function(data, status, headers, config) {
console.log('Error %o %o', status, data.message);
Expand Down Expand Up @@ -275,6 +355,10 @@ angular.module('zeppelinWebApp').controller('InterpreterCtrl',
if (newSetting.depArtifact !== '' || newSetting.depArtifact) {
$scope.addNewInterpreterDependency();
}
if (newSetting.option.setPermission === undefined) {
newSetting.option.setPermission = false;
}
newSetting.option.users = angular.element('#newInterpreterUsers').val();

var request = angular.copy($scope.newInterpreterSetting);

Expand Down Expand Up @@ -311,6 +395,7 @@ angular.module('zeppelinWebApp').controller('InterpreterCtrl',
option: {
remote: true,
isExistingProcess: false,
setPermission: false,
perNoteSession: false,
perNoteProcess: false

Expand Down Expand Up @@ -487,6 +572,7 @@ angular.module('zeppelinWebApp').controller('InterpreterCtrl',
var init = function() {
$scope.resetNewInterpreterSetting();
$scope.resetNewRepositorySetting();

getInterpreterSettings();
getAvailableInterpreters();
getRepositories();
Expand Down
8 changes: 8 additions & 0 deletions zeppelin-web/src/app/interpreter/interpreter.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,14 @@
overflow-y: auto;
}

.permissionsForm {
list-style-type: none;
background: #EFEFEF;
padding: 10px 10px 10px 10px;
box-shadow: 0 1px 1px rgba(0, 0, 0, 0.15);
border: 1px solid #E5E5E5;
}

.interpreterSettingAdd {
margin : 5px 5px 5px 5px;
padding : 10px 10px 10px 10px;
Expand Down
29 changes: 28 additions & 1 deletion zeppelin-web/src/app/interpreter/interpreter.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,10 @@ <h4>Repositories</h4>
</div>

<div class="box width-full"
ng-repeat="setting in interpreterSettings | orderBy: 'name' | filter: searchInterpreter">
ng-repeat="setting in interpreterSettings | orderBy: 'name' | filter: searchInterpreter" interpreter-directive>
<div id="{{setting.name | lowercase}}">
<div class="row interpreter">

<div class="col-md-12">
<h3 class="interpreter-title">{{setting.name}}
<small>
Expand Down Expand Up @@ -191,7 +192,33 @@ <h5>Option</h5>
pu-elastic-input-minwidth="180px" ng-model="setting.option.port" ng-disabled="!valueform.$visible" />
</div>

<div class="col-md-12">
<div class="checkbox">
<span class="input-group" style="line-height:30px;">
<label><input type="checkbox" style="width:18px !important" id="idShowPermission" ng-click="togglePermissions(setting.name)" ng-model="setting.option.setPermission" ng-disabled="!valueform.$visible"/>
Set permission </label>
</span>
</div>
</div>

<div class="col-md-12">
<!-- permissions -->
<div ng-show="setting.option.setPermission" class="permissionsForm">
<div>
<p>
Enter comma separated users in the fields. <br />
Empty field (*) implies anyone can run this interpreter.
</p>
<div>

<span class="owners">Owners </span>
<select id="{{setting.name}}Users" class="form-control" multiple="multiple" ng-disabled="!valueform.$visible">
<option ng-repeat="user in setting.option.users" selected="selected">{{user}}</option>
</select>
</div>
</div>
</div>
</div>

<div ng-show="_.isEmpty(setting.properties) && _.isEmpty(setting.dependencies) || valueform.$hidden" class="col-md-12 gray40-message">
<em>Currently there are no properties and dependencies set for this interpreter</em>
Expand Down
16 changes: 15 additions & 1 deletion zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterOption.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package org.apache.zeppelin.interpreter;

import java.util.List;

/**
*
*/
Expand All @@ -28,7 +30,8 @@ public class InterpreterOption {
boolean perNoteProcess;

boolean isExistingProcess;

boolean setPermission;
List<String> users;

public boolean isExistingProcess() {
return isExistingProcess;
Expand All @@ -46,6 +49,17 @@ public void setHost(String host) {
this.host = host;
}

public boolean isSetPermission() {

@bzz bzz Aug 11, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite an ambiguous naming - usually in Java set\get used as a convention for getters and setters, so may be here we better call it like permissionIsSet() or permissionWasSet() - in this way we keep the almos-eglish readability in case of the code like

if (permissionIsSet()) {
 //....
}

return setPermission;
}

public void setUserPermission(boolean setPermission) {
this.setPermission = setPermission;
}

public List<String> getUsers() {
return users;
}

public InterpreterOption() {
remote = false;
Expand Down
31 changes: 31 additions & 0 deletions zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Paragraph.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,19 @@ public Map<String, Object> info() {
return null;
}

private boolean hasPermission(String user, List<String> intpUsers) {
if (1 > intpUsers.size()) {
return true;
}

for (String u: intpUsers) {
if (user.trim().equals(u.trim())) {
return true;
}
}
return false;
}

@Override
protected Object jobRun() throws Throwable {
String replName = getRequiredReplName();
Expand All @@ -285,6 +298,24 @@ protected Object jobRun() throws Throwable {
throw new RuntimeException("Can not find interpreter for " + getRequiredReplName());
}

if (this.user != null &&
!factory.getInterpreterSettings(note.getId()).isEmpty()) {
for (InterpreterSetting intp: factory.getInterpreterSettings(note.getId())){

if (replName.startsWith(intp.getName()) &&
intp.getOption().isSetPermission() &&
!hasPermission(authenticationInfo.getUser(), intp.getOption().getUsers())) {
logger.error("{} has no permission for {} ", authenticationInfo.getUser(), repl);

@bzz bzz Aug 11, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to give you a perspective of somebody who reads this code: it is VERY hard to understand and reason what happens in both conditions here.

By looking at first condition:

this.user != null && !factory.getInterpreterSettings(note.getId()).isEmpty()

I would assume that is some kind of pre-condition check, because we do not do anything in case it is false.
But if that is true, we do something for each interpreter.

Then I would look at both conditions to understand, what they mean, starting from this.user != null. As a reader, I have no idea what is the case when this.user can be null. Is that because of some kind of error condition? Or is that because auth is not enabled? Or is that the case when user is not authorized? It's very hard to say, without spending a lot of time to understand all the cases

Same for factory.getInterpreterSettings(note.getId()).isEmpty() - what does it mean when interpreter settings are empty for a note? When can that happen? Why do we check for it here? Ah, after some thinking and looking for other code that uses it - I can see, that is some kind of collection!

And all this questions for reader could be avoided, and time saved, if we give a reader a clue in form of readable names for this conditions in english - we can extract this logic to the functions that self-describe it's purpose through the function names (you can think of it as a form of documentation):

if (this.noteHasUser() && this.noteHasInterpreters()) {
  //...
}

boolean noteHasUser() {
  return this.user != null;
}

boolean noteHasInterpreters() {
  // BTW, this is used in 2 other places and looks like candidate for further simplification
  // return !getInterpreterListFor(this.note).isEmpty();
  return !factory.getInterpreyterSettings(note.getId()).isEmpty();
}

By looking at second condition:

replName.startsWith(intp.getName()) &&
 +          intp.getOption().isSetPermission() &&
 +          !hasPermission(authenticationInfo.getUser(), intp.getOption().getUsers())

as a reader, for me it's even harder to say, what case does this represent.

Judging by the loop and an error message - it looks this code does to things:

  • finds current interpreter by name
  • only for the current interpreter, it checks if user has access to it

So a function can be extracted here as well:

boolean isUserAuthorizedToAccessInterpreter(String user,  InterpreterOption intpOpt)){
  return intpOpt.isSetPermission() &&
         !hasPermission(authenticationInfo.getUser(), intpOpt.getUsers())
}

But it can be even further simplified if we decouple logic for finding current interp in collection by replName to a new function findInerpreterByName (from the logic for checking if user is authorized to access the interp):

InterpreterSettings findInerpreterByName(String name) {
  InterpreterSettings intp = null; // this would rather be `Option()` or `InterpreterSettings.NONE` or anything else type-safe
  for (InterpreterSetting i: factory.getInterpreterSettings(note.getId())) { // could re-use `getInterpreterListFor(this.note)` from above
    if (i.getName().startsWith(name)) {
      interp = i;
    }
 }
 return intp;
}

So, final results

Before

if (this.user != null &&
  !factory.getInterpreterSettings(note.getId()).isEmpty()) {
  for (InterpreterSetting intp: factory.getInterpreterSettings(note.getId())){

    if (replName.startsWith(intp.getName()) &&
      intp.getOption().isSetPermission() &&
      !hasPermission(authenticationInfo.getUser(), intp.getOption().getUsers())) {
      return new InterpreterResult(...);
    }
 }
}

After

if (this.noteHasUser() && this.noteHasInterpreters()) {
  InterpreterSetting intp = findInerpreterByName(replName);
  if (intp != null && 
      isUserAuthorizedToAccessInterpreter(authenticationInfo.getUser(), intp.getOption())) {
    return new InterpreterResult(...);
  }
}

I'm not native english speaker, but the resulting code very much looks like an english sentence. It explains the logic for the reader, who can follow the implementation details by looking at the new functions.

What do you think, does this make sense?

@astroshim astroshim Aug 11, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All your comments are totally makes sense and thank you very much for the detail review to spend your precious time.
I'll fix them.
Thank you again.

return new InterpreterResult(Code.ERROR, authenticationInfo.getUser() +
" has no permission for " + getRequiredReplName());
/*
throw new RuntimeException(authenticationInfo.getUser() +
" has no permission for " + getRequiredReplName());
*/

@prabhjyotsingh prabhjyotsingh Aug 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is not required can you remove it.

@astroshim astroshim Aug 9, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted it. Thanks.

}
}
}

String script = getScriptBody();
// inject form
if (repl.getFormType() == FormType.NATIVE) {
Expand Down