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 3 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
162 changes: 162 additions & 0 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 @@ -22,6 +22,164 @@ angular.module('zeppelinWebApp').controller('InterpreterCtrl',
$scope.showRepositoryInfo = false;
$scope._ = _;

// auto complete related
$scope.suggestions = [];
$scope.selectIndex = -1;
var selectedUser = '';
var selectedUserIndex = 0;
var previousSelectedList = [];
var searchText = [];

function updatePreviousList() {

@heruka-urgyen heruka-urgyen Aug 4, 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.

updatePreviousList defined multiple times; for loop inside (or even the whole function) can be replaced with _.clone

for (var i = 0; i < searchText.length; i++) {
previousSelectedList[i] = searchText[i];
}
}

function convertToString(setting) {

@heruka-urgyen heruka-urgyen Aug 4, 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.

the name convertToString implies the following signature Array -> String, at least in my opinion. This implementation, however as a signature Object -> Void which makes it hard for reader to understand what's happening. a more appropriate name would be something like assignToUsers; it should also at least receive stringified searchText as an argument

setting.option.users = searchText.join();
}

var checkIfSelected = function() {
if (($scope.suggestions.length === 0) &&
($scope.selectIndex < 0 || $scope.selectIndex >= $scope.suggestions.length) ||
($scope.suggestions.length !== 0 && ($scope.selectIndex < 0 || $scope.selectIndex >= $scope.suggestions.length))

@bzz bzz Aug 4, 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.

Long condition inside the if becomes quite hard to understand and follow here.

What do you think, could this be changed a bit to make it more readable i.e:

if (suggestionSelected($scope.suggestions.length, $scope.selectIndex)) {
 //....
}

function suggestionSelected(len, i) {
  return (len === 0) && (i < 0 || i >= len) || 
         (len !== 0  && (i < 0 || i >= len))
}

And now it's easy to see how suggestionSelected can be further improved, keeping invariants.

Normalizing the parantheses according to logical operator precedence in javascript we get

return (len === 0 && (i < 0 || i >= len)) || 
       (len !== 0 && (i < 0 || i >= len))
}

which, in turn, is the same as

return (i < 0 || i >= len) && (len === 0 || len !== 0)

and as the right side of the && is always true for an integer, we get

function suggestionSelected(len, i) {
 return i < 0 || i >= len
}

At the end, this might not worth extracting to a separate function, but this shows the benefits we can get from it.
The result is much simpler to understand and reason about.

@astroshim what do you think?

) {
searchText[selectedUserIndex] = selectedUser;
$scope.suggestions = [];

@heruka-urgyen heruka-urgyen Aug 4, 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.

this is a boolean function, however it mutates two things inside its body; shall these mutations be factored out to the outer scope?

return true;
} else {
return false;
}
};

$scope.checkKeyDown = function(event, setting) {
if (event.keyCode === 40) {
event.preventDefault();
if ($scope.selectIndex + 1 !== $scope.suggestions.length) {

@heruka-urgyen heruka-urgyen Aug 4, 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.

I think $scope.selectIndex < $scope.suggestions.length would be more clear

$scope.selectIndex++;
}
} else if (event.keyCode === 38) {
event.preventDefault();

if ($scope.selectIndex - 1 !== -1) {

@heruka-urgyen heruka-urgyen Aug 4, 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.

$scope.selectIndex > 0

$scope.selectIndex--;

}
} else if (event.keyCode === 13) {
event.preventDefault();
if (!checkIfSelected()) {
selectedUser = $scope.suggestions[$scope.selectIndex];
searchText[selectedUserIndex] = $scope.suggestions[$scope.selectIndex];
updatePreviousList();
convertToString(setting);
$scope.suggestions = [];
}
}

};

$scope.checkKeyUp = function(event, setting) {
if (event.keyCode !== 8 || event.keyCode !== 46) {
if (searchText[selectedUserIndex] === '') {
$scope.suggestions = [];
}
}
if (event.keyCode < 37 || event.keyCode > 40) { // arrow buttons.
$scope.search(setting.option.users, setting);
}
};

$scope.assignValueAndHide = function(index, setting) {
searchText[selectedUserIndex] = $scope.suggestions[index];
updatePreviousList();
convertToString(setting);
$scope.suggestions = [];
};

angular.element(document).click(function() {
angular.element('.userlist').hide();
angular.element('.ace_autocomplete').hide();
});

function getSuggestions(searchQuery) {
$scope.suggestions = [];
$http.get(baseUrlSrv.getRestApiBase() + '/security/userlist/' + searchQuery).then(function
(response) {
var userlist = angular.fromJson(response.data).body;

for (var k in userlist) {
$scope.suggestions.push(userlist[k]);
}
});
}

function updatePreviousList() {
for (var i = 0; i < searchText.length; i++) {
previousSelectedList[i] = searchText[i];
}
}

var getChangedIndex = function() {
if (previousSelectedList.length === 0) {
selectedUserIndex = searchText.length - 1;
} else {
for (var i = 0; i < searchText.length; i++) {
if (previousSelectedList[i] !== searchText[i]) {
selectedUserIndex = i;
previousSelectedList = [];
break;
}
}
}
updatePreviousList();
};

function convertToArray(setting) {
if (!setting.option.users) {
return;
} else if (typeof setting.option.users === 'string') {
searchText = setting.option.users.split(',');
}

for (var i = 0; i < searchText.length; i++) {
searchText[i] = searchText[i].trim();
}
}

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

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

$scope.togglePermissions = function() {
if ($scope.showInterpreterAuth) {
$scope.closePermissions();
} else {
$scope.openPermissions();
}
};

// function to find suggestion list on change
$scope.search = function(searchQuery, setting) {
angular.element('.userlist').show();
convertToArray(setting);

getChangedIndex();
$scope.selectIndex = -1;
$scope.suggestions = [];
selectedUser = searchText[selectedUserIndex];

if (selectedUser !== '') {
getSuggestions(selectedUser);
} else {
$scope.suggestions = [];
}
};

var getInterpreterSettings = function() {
$http.get(baseUrlSrv.getRestApiBase() + '/interpreter/setting').success(function(data, status, headers, config) {
$scope.interpreterSettings = data.body;
Expand Down Expand Up @@ -148,6 +306,9 @@ 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;
Expand Down Expand Up @@ -311,6 +472,7 @@ angular.module('zeppelinWebApp').controller('InterpreterCtrl',
option: {
remote: true,
isExistingProcess: false,
setPermission: false,
perNoteSession: false,
perNoteProcess: false

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
42 changes: 42 additions & 0 deletions zeppelin-web/src/app/interpreter/interpreter.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,48 @@ <h5>Option</h5>
</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()" 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 interpreters in the fields. <br />

@AhyoungRyu AhyoungRyu Aug 4, 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.

Don't we need to type comma separated users list not interpreters in this field?

Empty field (*) implies anyone can run the interpreters.

@AhyoungRyu AhyoungRyu Aug 4, 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.

It's a minor thing; how about this interpreter instead of the interpreters?

</p>
<div>
<form class="form-inline">
<div class="form-group">
<label for="User">Users</label>
<input type="text" class="form-control"
ng-model="setting.option.users"
placeholder="search for users"
ng-keydown="checkKeyDown($event, setting)"
ng-keyup="checkKeyUp($event, setting)"
ng-disabled="!valueform.$visible"/>
</div>
<div class="userlist" >
<ul>
<li ng-repeat="suggestion in suggestions"
ng-class="{active : selectIndex === $index}"
ng-click="assignValueAndHide($index, setting)">
{{suggestion}}
</li>
</ul>
</div>
</form>
</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
17 changes: 16 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 @@ -28,7 +28,8 @@ public class InterpreterOption {
boolean perNoteProcess;

boolean isExistingProcess;

boolean setPermission;
String users;

public boolean isExistingProcess() {
return isExistingProcess;
Expand All @@ -46,6 +47,20 @@ 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 void setUsers(String users) {
this.users = users;
}
public 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,20 @@ public Map<String, Object> info() {
return null;
}

private boolean hasPermission(String user, String intpUsers) {
if (intpUsers.trim().equals("")) {
return true;
}

String[] userList = intpUsers.split(",");
for (String u: userList) {
if (user.trim().equals(u.trim())) {
return true;
}
}
return false;
}

@Override
protected Object jobRun() throws Throwable {
String replName = getRequiredReplName();
Expand All @@ -285,6 +299,23 @@ 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