From 9e0f85a1ff941577c99a3f992facaf750d1913bd Mon Sep 17 00:00:00 2001 From: Nelson Costa Date: Fri, 6 Apr 2018 09:44:07 +0100 Subject: [PATCH] [ZEPPELIN-3168] Interpreter Settings Authorization --- docs/usage/interpreter/overview.md | 17 +++ .../zeppelin/conf/ZeppelinConfiguration.java | 1 - .../DefaultInterpreterProperty.java | 29 +++-- .../interpreter/InterpreterOption.java | 103 ++++++++++----- .../interpreter/InterpreterProperty.java | 28 +++- .../zeppelin/rest/InterpreterRestApi.java | 55 +++++++- .../zeppelin/rest/AbstractTestRestApi.java | 14 +- .../app/interpreter/interpreter-create.html | 52 +++++--- .../app/interpreter/interpreter.controller.js | 80 +++++++++--- .../src/app/interpreter/interpreter.html | 120 +++++++++++++----- zeppelin-web/src/app/notebook/notebook.html | 4 +- .../interpreter/InterpreterAuthorization.java | 93 ++++++++++++++ .../interpreter/InterpreterInfoSaving.java | 7 +- .../interpreter/InterpreterSetting.java | 51 +++----- .../InterpreterSettingManager.java | 33 +++-- .../apache/zeppelin/notebook/Paragraph.java | 2 +- .../zeppelin/storage/ConfigStorage.java | 3 +- .../storage/FileSystemConfigStorage.java | 7 - 18 files changed, 521 insertions(+), 178 deletions(-) create mode 100644 zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterAuthorization.java diff --git a/docs/usage/interpreter/overview.md b/docs/usage/interpreter/overview.md index 035c381b8a9..9b059050e4c 100644 --- a/docs/usage/interpreter/overview.md +++ b/docs/usage/interpreter/overview.md @@ -152,3 +152,20 @@ In such cases, interpreter process recovery is necessary. Starting from 0.8.0, u `org.apache.zeppelin.interpreter.recovery.FileSystemRecoveryStorage` or other implementations if available in future, by default it is `org.apache.zeppelin.interpreter.recovery.NullRecoveryStorage` which means recovery is not enabled. Enable recover means shutting down Zeppelin would not terminating interpreter process, and when Zeppelin is restarted, it would try to reconnect to the existing running interpreter processes. If you want to kill all the interpreter processes after terminating Zeppelin even when recovery is enabled, you can run `bin/stop-interpreter.sh` + +## Interpreter permissions + +Zeppelin's interpreter settings contain 2 levels of permissions: +* Owners - Can run/read/edit/restart an interpreter setting +* Readers - Cannot edit an interpreter setting + +These settings can be set directly on `interpreter.json` or in the Interpreter Settings page, `Set permissions` option. + +## Interpreter properties + +Zeppelin's interpreter properties allow the setting of the following attributes: +* Name - Name of the property +* Value - Value of the property +* Type - Value type of the property (number, string, url, ...) +* Readonly - Flag indicating if property is editable or not in UI +* Description - Description of the property diff --git a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java index 2960cd03faa..00a96c70d78 100644 --- a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java +++ b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java @@ -798,7 +798,6 @@ public enum ConfVars { ZEPPELIN_NOTEBOOK_GIT_REMOTE_ORIGIN("zeppelin.notebook.git.remote.origin", "origin"), ZEPPELIN_NOTEBOOK_CRON_ENABLE("zeppelin.notebook.cron.enable", false), ZEPPELIN_NOTEBOOK_CRON_FOLDERS("zeppelin.notebook.cron.folders", null); - private String varName; @SuppressWarnings("rawtypes") private Class varClass; diff --git a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/DefaultInterpreterProperty.java b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/DefaultInterpreterProperty.java index f11cbc37b1a..d0f69e7a9d3 100644 --- a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/DefaultInterpreterProperty.java +++ b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/DefaultInterpreterProperty.java @@ -18,7 +18,7 @@ package org.apache.zeppelin.interpreter; /** - * Property for registered interpreter + * Registered interpreter property object (interpreter-setting.json) */ public class DefaultInterpreterProperty { private String envName; @@ -26,32 +26,36 @@ public class DefaultInterpreterProperty { private Object defaultValue; private String description; private String type; + private Boolean readonly; public DefaultInterpreterProperty(String envName, String propertyName, Object defaultValue, - String description, String type) { + String description, String type, Boolean readonly) { this.envName = envName; this.propertyName = propertyName; this.defaultValue = defaultValue; this.description = description; this.type = type; + this.readonly = readonly; } public DefaultInterpreterProperty(Object defaultValue, String description, String type) { - this(null, null, defaultValue, description, type); + this(null, null, defaultValue, description, type, false); } public DefaultInterpreterProperty(Object defaultValue, String description) { - this(null, null, defaultValue, description, InterpreterPropertyType.TEXTAREA.getValue()); + this(null, null, defaultValue, description, + InterpreterPropertyType.TEXTAREA.getValue(), false); } public DefaultInterpreterProperty(String envName, String propertyName, String defaultValue) { - this(envName, propertyName, defaultValue, null, InterpreterPropertyType.TEXTAREA.getValue()); + this(envName, propertyName, defaultValue, null, + InterpreterPropertyType.TEXTAREA.getValue(), false); } public DefaultInterpreterProperty(String envName, String propertyName, String defaultValue, String description) { this(envName, propertyName, defaultValue, description, - InterpreterPropertyType.TEXTAREA.getValue()); + InterpreterPropertyType.TEXTAREA.getValue(), false); } public String getEnvName() { @@ -94,6 +98,14 @@ public void setType(String type) { this.type = type; } + public Boolean getReadonly() { + return readonly; + } + + public void setReadonly(Boolean readonly) { + this.readonly = readonly; + } + public int hashCode() { return this.toString().hashCode(); } @@ -122,7 +134,8 @@ public Object getValue() { @Override public String toString() { - return String.format("{envName=%s, propertyName=%s, defaultValue=%s, description=%20s, " + - "type=%s}", envName, propertyName, defaultValue, description, type); + return String.format("{envName=%s, propertyName=%s, defaultValue=%s, " + + "description=%20s, type=%s, readonly=%s}", + envName, propertyName, defaultValue, description, type, readonly); } } diff --git a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOption.java b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOption.java index e8a92255fc9..f4ab42c408e 100644 --- a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOption.java +++ b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOption.java @@ -17,11 +17,15 @@ package org.apache.zeppelin.interpreter; -import java.util.ArrayList; -import java.util.List; +import com.google.gson.JsonArray; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; + +import java.util.Set; +import java.util.HashSet; /** - * + * Interpreter options object */ public class InterpreterOption { public static final transient String SHARED = "shared"; @@ -30,23 +34,35 @@ public class InterpreterOption { // always set it as true, keep this field just for backward compatibility boolean remote = true; - String host = null; - int port = -1; + private String host = null; + private int port = -1; String perNote; String perUser; boolean isExistingProcess; - boolean setPermission; - List owners; - boolean isUserImpersonate; + private boolean setPermission; + private Set owners; + private Set readers; + private boolean isUserImpersonate; + private boolean disallowCustomInterpreter; - public boolean isExistingProcess() { - return isExistingProcess; + public InterpreterOption() { } + + public InterpreterOption(String perUser, String perNote) { + if (perUser == null) { + throw new NullPointerException("perUser can not be null."); + } + if (perNote == null) { + throw new NullPointerException("perNote can not be null."); + } + + this.perUser = perUser; + this.perNote = perNote; } - public void setExistingProcess(boolean isExistingProcess) { - this.isExistingProcess = isExistingProcess; + public boolean isExistingProcess() { + return isExistingProcess; } public void setPort(int port) { @@ -61,12 +77,12 @@ public boolean permissionIsSet() { return setPermission; } - public void setUserPermission(boolean setPermission) { - this.setPermission = setPermission; + public Set getOwners() { + return owners; } - public List getOwners() { - return owners; + public Set getReaders() { + return readers; } public boolean isUserImpersonate() { @@ -77,19 +93,12 @@ public void setUserImpersonate(boolean userImpersonate) { isUserImpersonate = userImpersonate; } - public InterpreterOption() { + public boolean getDisallowCustomInterpreter() { + return disallowCustomInterpreter; } - public InterpreterOption(String perUser, String perNote) { - if (perUser == null) { - throw new NullPointerException("perUser can not be null."); - } - if (perNote == null) { - throw new NullPointerException("perNote can not be null."); - } - - this.perUser = perUser; - this.perNote = perNote; + public void setDisallowCustomInterpreter(boolean customInterpreter) { + disallowCustomInterpreter = customInterpreter; } public static InterpreterOption fromInterpreterOption(InterpreterOption other) { @@ -102,8 +111,9 @@ public static InterpreterOption fromInterpreterOption(InterpreterOption other) { option.isExistingProcess = other.isExistingProcess; option.setPermission = other.setPermission; option.owners = (null == other.owners) ? - new ArrayList() : new ArrayList<>(other.owners); - + new HashSet() : new HashSet<>(other.owners); + option.readers = (null == other.readers) ? + new HashSet() : new HashSet<>(other.readers); return option; } @@ -115,7 +125,6 @@ public int getPort() { return port; } - public boolean perUserShared() { return SHARED.equals(perUser); } @@ -155,4 +164,38 @@ public void setPerNote(String perNote) { public void setPerUser(String perUser) { this.perUser = perUser; } + + public void convertToOwners(JsonObject jsonObject) { + if (jsonObject != null) { + JsonObject option = jsonObject.getAsJsonObject("option"); + if (option != null) { + JsonArray users = option.getAsJsonArray("users"); + if (users != null) { + if (this.getOwners() == null) { + this.owners = new HashSet<>(); + } + for (JsonElement user : users) { + this.getOwners().add(user.getAsString()); + } + } + } + } + } + + public void convertToReaders(JsonObject jsonObject) { + if (jsonObject != null) { + JsonObject option = jsonObject.getAsJsonObject("option"); + if (option != null) { + JsonArray users = option.getAsJsonArray("users"); + if (users != null) { + if (this.getReaders() == null) { + this.readers = new HashSet<>(); + } + for (JsonElement user : users) { + this.getReaders().add(user.getAsString()); + } + } + } + } + } } diff --git a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterProperty.java b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterProperty.java index 92cf3a8febc..e0ff5e6b3a3 100644 --- a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterProperty.java +++ b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterProperty.java @@ -18,17 +18,22 @@ package org.apache.zeppelin.interpreter; /** - * Property for instance of interpreter + * Interpreter property object (interpreter.json) */ public class InterpreterProperty { private String name; private Object value; private String type; + private Boolean readonly; + private String description; - public InterpreterProperty(String name, Object value, String type) { + public InterpreterProperty(String name, Object value, String type, + Boolean readonly, String description) { this.name = name; this.value = value; this.type = type; + this.readonly = readonly; + this.description = description; } public InterpreterProperty(String name, Object value) { @@ -61,8 +66,25 @@ public void setType(String type) { this.type = type; } + public Boolean getReadonly() { + return readonly; + } + + public void setReadonly(Boolean readonly) { + this.readonly = readonly; + } + + public String getDescription() { + return description; + } + + public void setDescription(String description) { + this.description = description; + } + @Override public String toString() { - return String.format("{name=%s, value=%s, type=%s}", name, value, type); + return String.format("{name=%s, value=%s, type=%s, readonly=%s}", + name, value, type, readonly); } } diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/InterpreterRestApi.java b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/InterpreterRestApi.java index 479740d6c8e..dd6817c9f41 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/InterpreterRestApi.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/InterpreterRestApi.java @@ -22,8 +22,11 @@ import org.sonatype.aether.repository.RemoteRepository; import java.io.IOException; +import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.ArrayList; import javax.servlet.http.HttpServletRequest; import javax.ws.rs.DELETE; @@ -37,12 +40,15 @@ import javax.ws.rs.core.Response; import javax.ws.rs.core.Response.Status; +import com.google.common.collect.Sets; import org.apache.zeppelin.annotation.ZeppelinApi; import org.apache.zeppelin.dep.Repository; +import org.apache.zeppelin.interpreter.InterpreterAuthorization; import org.apache.zeppelin.interpreter.InterpreterException; import org.apache.zeppelin.interpreter.InterpreterPropertyType; import org.apache.zeppelin.interpreter.InterpreterSetting; import org.apache.zeppelin.interpreter.InterpreterSettingManager; +import org.apache.zeppelin.rest.exception.ForbiddenException; import org.apache.zeppelin.rest.message.NewInterpreterSettingRequest; import org.apache.zeppelin.rest.message.RestartInterpreterRequest; import org.apache.zeppelin.rest.message.UpdateInterpreterSettingRequest; @@ -60,6 +66,7 @@ public class InterpreterRestApi { private InterpreterSettingManager interpreterSettingManager; private NotebookServer notebookServer; + private InterpreterAuthorization interpreterAuthorization; public InterpreterRestApi() { } @@ -68,6 +75,7 @@ public InterpreterRestApi(InterpreterSettingManager interpreterSettingManager, NotebookServer notebookWsServer) { this.interpreterSettingManager = interpreterSettingManager; this.notebookServer = notebookWsServer; + this.interpreterAuthorization = new InterpreterAuthorization(interpreterSettingManager); } /** @@ -77,7 +85,17 @@ public InterpreterRestApi(InterpreterSettingManager interpreterSettingManager, @Path("setting") @ZeppelinApi public Response listSettings() { - return new JsonResponse<>(Status.OK, "", interpreterSettingManager.get()).build(); + List settings = interpreterSettingManager.get(); + List filteredSettings = new ArrayList<>(); + Iterator it = settings.iterator(); + Set userAndRoles = getUserAndRoles(); + while (it.hasNext()) { + InterpreterSetting c = it.next(); + if (interpreterAuthorization.hasReadAuthorization(userAndRoles, c.getId())) { + filteredSettings.add(c); + } + } + return new JsonResponse<>(Status.OK, "", filteredSettings).build(); } /** @@ -89,6 +107,9 @@ public Response listSettings() { public Response getSetting(@PathParam("settingId") String settingId) { try { InterpreterSetting setting = interpreterSettingManager.get(settingId); + + checkIfUserCanRead(settingId, "No permissions to get interpreter setting"); + if (setting == null) { return new JsonResponse<>(Status.NOT_FOUND).build(); } else { @@ -136,6 +157,8 @@ public Response updateSetting(String message, @PathParam("settingId") String set logger.info("Update interpreterSetting {}", settingId); try { + checkIfUserCanWrite(settingId, "No permissions to update interpreter setting"); + UpdateInterpreterSettingRequest request = UpdateInterpreterSettingRequest.fromJson(message); interpreterSettingManager @@ -164,6 +187,7 @@ public Response updateSetting(String message, @PathParam("settingId") String set @Path("setting/{settingId}") @ZeppelinApi public Response removeSetting(@PathParam("settingId") String settingId) throws IOException { + checkIfUserCanWrite(settingId, "No permissions to remove interpreter setting"); logger.info("Remove interpreterSetting {}", settingId); interpreterSettingManager.remove(settingId); return new JsonResponse(Status.OK).build(); @@ -256,8 +280,13 @@ public Response getMetaInfo(@Context HttpServletRequest req, if (interpreterSetting == null) { return new JsonResponse<>(Status.NOT_FOUND).build(); } - Map infos = interpreterSetting.getInfos(); - return new JsonResponse<>(Status.OK, "metadata", infos).build(); + Map infosAll = interpreterSetting.getInfos(); + Set userAndRoles = getUserAndRoles(); + infosAll.put("canRead", String.valueOf(interpreterAuthorization + .hasReadAuthorization(userAndRoles, settingId))); + infosAll.put("canWrite", String.valueOf(interpreterAuthorization + .hasWriteAuthorization(userAndRoles, settingId))); + return new JsonResponse<>(Status.OK, "metadata", infosAll).build(); } /** @@ -288,4 +317,24 @@ public Response removeRepository(@PathParam("repoId") String repoId) { public Response listInterpreterPropertyTypes() { return new JsonResponse<>(Status.OK, InterpreterPropertyType.getTypes()).build(); } + private void checkIfUserCanRead(String settingId, String errorMsg) { + Set userAndRoles = getUserAndRoles(); + if (!interpreterAuthorization.hasReadAuthorization(userAndRoles, settingId)) { + throw new ForbiddenException(errorMsg); + } + } + + private void checkIfUserCanWrite(String settingId, String errorMsg) { + Set userAndRoles = getUserAndRoles(); + if (!interpreterAuthorization.hasWriteAuthorization(userAndRoles, settingId)) { + throw new ForbiddenException(errorMsg); + } + } + + private Set getUserAndRoles() { + Set userAndRoles = Sets.newHashSet(); + userAndRoles.add(SecurityUtils.getPrincipal()); + userAndRoles.addAll(SecurityUtils.getRoles()); + return userAndRoles; + } } diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/AbstractTestRestApi.java b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/AbstractTestRestApi.java index 172f1175ecd..cfd112dcd82 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/AbstractTestRestApi.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/AbstractTestRestApi.java @@ -58,10 +58,10 @@ public abstract class AbstractTestRestApi { protected static final Logger LOG = LoggerFactory.getLogger(AbstractTestRestApi.class); - static final String REST_API_URL = "/api"; - static final String URL = getUrlToTest(); - protected static final boolean WAS_RUNNING = checkIfServerIsRunning(); - static boolean isRunningWithAuth = false; + private static final String REST_API_URL = "/api"; + private static final String URL = getUrlToTest(); + private static final boolean WAS_RUNNING = checkIfServerIsRunning(); + private static boolean isRunningWithAuth = false; private static File shiroIni = null; private static String zeppelinShiro = @@ -126,8 +126,8 @@ public abstract class AbstractTestRestApi { + "/bA8TFNPblPxavIOcd+R+RfFmT1YKfYIhco=\n" + "-----END CERTIFICATE-----"; - protected static File zeppelinHome; - protected static File confDir; + private static File zeppelinHome; + private static File confDir; private String getUrl(String path) { String url; @@ -152,7 +152,7 @@ protected static String getUrlToTest() { return url; } - static ExecutorService executor; + private static ExecutorService executor; protected static final Runnable SERVER = new Runnable() { @Override public void run() { diff --git a/zeppelin-web/src/app/interpreter/interpreter-create.html b/zeppelin-web/src/app/interpreter/interpreter-create.html index 8bf29a91f43..ebe61825632 100644 --- a/zeppelin-web/src/app/interpreter/interpreter-create.html +++ b/zeppelin-web/src/app/interpreter/interpreter-create.html @@ -235,12 +235,13 @@
Option
- - - + + +
@@ -251,16 +252,27 @@
Option
-

- Enter comma separated users and groups in the fields.
- Empty field (*) implies anyone can run this interpreter. +

Enter comma separated users/roles in the fields.
+ Empty field implies no restrictions.

- Owners + + Owners: +
+
+ + Readers: + + +
@@ -272,8 +284,9 @@
Properties
name value - action + read-only description + action {{key}} @@ -292,14 +305,17 @@
Properties
type="checkbox" msd-elastic ng-model="newInterpreterSetting.properties[key].value" /> - + {{newInterpreterSetting.properties[key].description}} + + + - Properties + + + + + + - Set permission + + Disallow custom interpreter + + + + + +
+
+
+ +
@@ -370,16 +386,27 @@
Option
-

- Enter comma separated users and groups in the fields.
- Empty field (*) implies anyone can run this interpreter. +

Enter comma separated users/roles in the fields.
+ Empty field implies no restrictions.

- - Owners - + + Owners: + + +
+
+
+ + Readers: + +
@@ -395,40 +422,61 @@
Properties
- - - + + + + + + + - + +
namevalueactionnamevalueread-onlydescriptionaction
{{key}} + e-ng-readonly="setting.properties[key].readonly" + editable-textarea="setting.properties[key].value" + e-form="valueform" e-msd-elastic> {{setting.properties[key].value | breakFilter}} - {{setting.properties[key].value}} + e-ng-readonly="setting.properties[key].readonly" + editable-text="setting.properties[key].value" + e-form="valueform" e-msd-elastic> + {{setting.properties[key].value}} - {{setting.properties[key].value}} + e-ng-readonly="setting.properties[key].readonly" + editable-text="setting.properties[key].value" + e-widget-number e-form="valueform" e-msd-elastic> + {{setting.properties[key].value}} + e-ng-readonly="setting.properties[key].readonly" + editable-text="setting.properties[key].value" + e-form="valueform" e-msd-elastic> {{setting.properties[key].value}} - {{setting.properties[key].value ? '***' : ''}} + e-ng-readonly="setting.properties[key].readonly" + editable-password="setting.properties[key].value" + e-form="valueform" e-msd-elastic> + {{setting.properties[key].value ? '***' : ''}} - {{setting.properties[key].value}} + e-ng-readonly="setting.properties[key].readonly" + editable-checkbox="setting.properties[key].value" + e-form="valueform" e-msd-elastic> + {{setting.properties[key].value}} + + {{setting.properties[key].description}} + @@ -439,7 +487,6 @@
Properties
 @@ -448,6 +495,13 @@
Properties
 + + + +