You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@zeppelin.apache.org by mo...@apache.org on 2016/04/30 00:26:43 UTC
incubator-zeppelin git commit: ZEPPELIN-825: Set appropriate default permissions and allow user to c…
Repository: incubator-zeppelin
Updated Branches:
refs/heads/master d1287d302 -> 18fa33a9b
ZEPPELIN-825: Set appropriate default permissions and allow user to c…
### What is this PR for?
This PR makes users who set only r/w permissions the effective owners of the note.
1. If you set readers, and writers and owners is empty, it is automatically set to the user requesting the change.
2. If you set writers, and owners is empty, it is set it to the user requesting the change.
It also fixes a bug that did not allow one to clear the permissions for a note.
### What type of PR is it?
Bug Fix, Improvement
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-825
### How should this be tested?
Outline the steps to test the PR here.
### Questions:
* Does this needs documentation? YES
…lear permissions
This change makes users who set permissions the effective owners of the note.
1. If you set readers, and writers and owners is empty, it is automatically set to the user requesting the change.
2. If you set writers, and owners is empty, it is set it to the user requesting the change.
Author: Rohan Ramakrishna <ro...@twitter.com>
Closes #849 from rohannr/rohanr/ZEPPELIN-818 and squashes the following commits:
7dfc9cf [Rohan Ramakrishna] Merge branch 'rohanr/ZEPPELIN-818' of github.com:rohannr/incubator-zeppelin into ZEPPELIN-825
ac3f39e [Rohan Ramakrishna] Merge branch 'rohanr/ZEPPELIN-818' of github.com:rohannr/incubator-zeppelin into ZEPPELIN-825
d8f4a83 [Rohan Ramakrishna] Merge branch 'rohanr/ZEPPELIN-818' of github.com:rohannr/incubator-zeppelin into ZEPPELIN-825
f457861 [Rohan Ramakrishna] Merge branch 'master' of https://github.com/apache/incubator-zeppelin into ZEPPELIN-825
e29b9df [Rohan Ramakrishna] ZEPPELIN-825: Set appropriate default permissions and allow user to clear permissions
083070d [Rohan Ramakrishna] ZEPPELIN-818: Set appropriate default permissions and allow user to clear permissions
Project: http://git-wip-us.apache.org/repos/asf/incubator-zeppelin/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-zeppelin/commit/18fa33a9
Tree: http://git-wip-us.apache.org/repos/asf/incubator-zeppelin/tree/18fa33a9
Diff: http://git-wip-us.apache.org/repos/asf/incubator-zeppelin/diff/18fa33a9
Branch: refs/heads/master
Commit: 18fa33a9b1127efd7b05980427fc61871b41f593
Parents: d1287d3
Author: Rohan Ramakrishna <ro...@twitter.com>
Authored: Tue Apr 26 12:23:18 2016 -0700
Committer: Lee moon soo <mo...@apache.org>
Committed: Fri Apr 29 15:26:56 2016 -0700
----------------------------------------------------------------------
.../apache/zeppelin/rest/NotebookRestApi.java | 27 +++-
.../zeppelin/rest/NotebookRestApiTest.java | 127 +++++++++++++++++++
.../notebook/NotebookAuthorization.java | 30 +----
.../apache/zeppelin/notebook/NotebookTest.java | 8 ++
4 files changed, 165 insertions(+), 27 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-zeppelin/blob/18fa33a9/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
----------------------------------------------------------------------
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
index 2796500..482ea78 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
@@ -50,6 +50,7 @@ import org.quartz.CronExpression;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.collect.Sets;
import com.google.gson.Gson;
import com.google.gson.reflect.TypeToken;
import com.google.gson.GsonBuilder;
@@ -127,9 +128,29 @@ public class NotebookRestApi {
return new JsonResponse<>(Status.FORBIDDEN, ownerPermissionError(userAndRoles,
notebookAuthorization.getOwners(noteId))).build();
}
- notebookAuthorization.setOwners(noteId, permMap.get("owners"));
- notebookAuthorization.setReaders(noteId, permMap.get("readers"));
- notebookAuthorization.setWriters(noteId, permMap.get("writers"));
+
+ HashSet readers = permMap.get("readers");
+ HashSet owners = permMap.get("owners");
+ HashSet writers = permMap.get("writers");
+ // Set readers, if writers and owners is empty -> set to user requesting the change
+ if (readers != null && !readers.isEmpty()) {
+ if (writers.isEmpty()) {
+ writers = Sets.newHashSet(SecurityUtils.getPrincipal());
+ }
+ if (owners.isEmpty()) {
+ owners = Sets.newHashSet(SecurityUtils.getPrincipal());
+ }
+ }
+ // Set writers, if owners is empty -> set to user requesting the change
+ if ( writers != null && !writers.isEmpty()) {
+ if (owners.isEmpty()) {
+ owners = Sets.newHashSet(SecurityUtils.getPrincipal());
+ }
+ }
+
+ notebookAuthorization.setReaders(noteId, readers);
+ notebookAuthorization.setWriters(noteId, writers);
+ notebookAuthorization.setOwners(noteId, owners);
LOG.debug("After set permissions {} {} {}",
notebookAuthorization.getOwners(noteId),
notebookAuthorization.getReaders(noteId),
http://git-wip-us.apache.org/repos/asf/incubator-zeppelin/blob/18fa33a9/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookRestApiTest.java
----------------------------------------------------------------------
diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookRestApiTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookRestApiTest.java
new file mode 100644
index 0000000..dc88b5d
--- /dev/null
+++ b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookRestApiTest.java
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.rest;
+
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+import com.google.gson.Gson;
+import com.google.gson.reflect.TypeToken;
+import org.apache.commons.httpclient.methods.GetMethod;
+import org.apache.commons.httpclient.methods.PutMethod;
+import org.apache.zeppelin.notebook.Note;
+import org.apache.zeppelin.notebook.NotebookAuthorization;
+import org.apache.zeppelin.notebook.NotebookAuthorizationInfoSaving;
+import org.apache.zeppelin.server.ZeppelinServer;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.FixMethodOrder;
+import org.junit.Test;
+import org.junit.runners.MethodSorters;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+
+/**
+ * Zeppelin notebook rest api tests
+ */
+@FixMethodOrder(MethodSorters.NAME_ASCENDING)
+public class NotebookRestApiTest extends AbstractTestRestApi {
+ Gson gson = new Gson();
+
+ @BeforeClass
+ public static void init() throws Exception {
+ AbstractTestRestApi.startUp();
+ }
+
+ @AfterClass
+ public static void destroy() throws Exception {
+ AbstractTestRestApi.shutDown();
+ }
+
+ @Test
+ public void testPermissions() throws IOException {
+ Note note1 = ZeppelinServer.notebook.createNote();
+ // Set only readers
+ String jsonRequest = "{\"readers\":[\"admin-team\"],\"owners\":[]," +
+ "\"writers\":[]}";
+ PutMethod put = httpPut("/notebook/" + note1.getId() + "/permissions/", jsonRequest);
+ LOG.info("testPermissions response\n" + put.getResponseBodyAsString());
+ assertThat("test update method:", put, isAllowed());
+ put.releaseConnection();
+
+
+ GetMethod get = httpGet("/notebook/" + note1.getId() + "/permissions/");
+ assertThat(get, isAllowed());
+ Map<String, Object> resp = gson.fromJson(get.getResponseBodyAsString(), new TypeToken<Map<String, Object>>() {
+ }.getType());
+ Map<String, Set<String>> authInfo = (Map<String, Set<String>>) resp.get("body");
+
+ // Check that both owners and writers is set to the princpal if empty
+ assertEquals(authInfo.get("readers"), Lists.newArrayList("admin-team"));
+ assertEquals(authInfo.get("owners"), Lists.newArrayList("anonymous"));
+ assertEquals(authInfo.get("writers"), Lists.newArrayList("anonymous"));
+ get.releaseConnection();
+
+
+ Note note2 = ZeppelinServer.notebook.createNote();
+ // Set only writers
+ jsonRequest = "{\"readers\":[],\"owners\":[]," +
+ "\"writers\":[\"admin-team\"]}";
+ put = httpPut("/notebook/" + note2.getId() + "/permissions/", jsonRequest);
+ assertThat("test update method:", put, isAllowed());
+ put.releaseConnection();
+
+ get = httpGet("/notebook/" + note2.getId() + "/permissions/");
+ assertThat(get, isAllowed());
+ resp = gson.fromJson(get.getResponseBodyAsString(), new TypeToken<Map<String, Object>>() {
+ }.getType());
+ authInfo = (Map<String, Set<String>>) resp.get("body");
+ // Check that owners is set to the princpal if empty
+ assertEquals(authInfo.get("owners"), Lists.newArrayList("anonymous"));
+ assertEquals(authInfo.get("writers"), Lists.newArrayList("admin-team"));
+ get.releaseConnection();
+
+
+ // Test clear permissions
+ jsonRequest = "{\"readers\":[],\"owners\":[],\"writers\":[]}";
+ put = httpPut("/notebook/" + note2.getId() + "/permissions/", jsonRequest);
+ put.releaseConnection();
+ get = httpGet("/notebook/" + note2.getId() + "/permissions/");
+ assertThat(get, isAllowed());
+ resp = gson.fromJson(get.getResponseBodyAsString(), new TypeToken<Map<String, Object>>() {
+ }.getType());
+ authInfo = (Map<String, Set<String>>) resp.get("body");
+
+ assertEquals(authInfo.get("readers"), Lists.newArrayList());
+ assertEquals(authInfo.get("writers"), Lists.newArrayList());
+ assertEquals(authInfo.get("owners"), Lists.newArrayList());
+ get.releaseConnection();
+ //cleanup
+ ZeppelinServer.notebook.removeNote(note1.getId());
+ ZeppelinServer.notebook.removeNote(note2.getId());
+
+ }
+}
+
+
http://git-wip-us.apache.org/repos/asf/incubator-zeppelin/blob/18fa33a9/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NotebookAuthorization.java
----------------------------------------------------------------------
diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NotebookAuthorization.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NotebookAuthorization.java
index 7efa46d..212d608 100644
--- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NotebookAuthorization.java
+++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NotebookAuthorization.java
@@ -110,16 +110,10 @@ public class NotebookAuthorization {
noteAuthInfo.put("owners", new LinkedHashSet(entities));
noteAuthInfo.put("readers", new LinkedHashSet());
noteAuthInfo.put("writers", new LinkedHashSet());
- authInfo.put(noteId, noteAuthInfo);
} else {
- Set<String> existingEntities = noteAuthInfo.get("owners");
- if (existingEntities == null) {
- noteAuthInfo.put("owners", new LinkedHashSet(entities));
- } else {
- existingEntities.clear();
- existingEntities.addAll(entities);
- }
+ noteAuthInfo.put("owners", new LinkedHashSet(entities));
}
+ authInfo.put(noteId, noteAuthInfo);
saveToFile();
}
@@ -130,16 +124,10 @@ public class NotebookAuthorization {
noteAuthInfo.put("owners", new LinkedHashSet());
noteAuthInfo.put("readers", new LinkedHashSet(entities));
noteAuthInfo.put("writers", new LinkedHashSet());
- authInfo.put(noteId, noteAuthInfo);
} else {
- Set<String> existingEntities = noteAuthInfo.get("readers");
- if (existingEntities == null) {
- noteAuthInfo.put("readers", new LinkedHashSet(entities));
- } else {
- existingEntities.clear();
- existingEntities.addAll(entities);
- }
+ noteAuthInfo.put("readers", new LinkedHashSet(entities));
}
+ authInfo.put(noteId, noteAuthInfo);
saveToFile();
}
@@ -150,16 +138,10 @@ public class NotebookAuthorization {
noteAuthInfo.put("owners", new LinkedHashSet());
noteAuthInfo.put("readers", new LinkedHashSet());
noteAuthInfo.put("writers", new LinkedHashSet(entities));
- authInfo.put(noteId, noteAuthInfo);
} else {
- Set<String> existingEntities = noteAuthInfo.get("writers");
- if (existingEntities == null) {
- noteAuthInfo.put("writers", new LinkedHashSet(entities));
- } else {
- existingEntities.clear();
- existingEntities.addAll(entities);
- }
+ noteAuthInfo.put("writers", new LinkedHashSet(entities));
}
+ authInfo.put(noteId, noteAuthInfo);
saveToFile();
}
http://git-wip-us.apache.org/repos/asf/incubator-zeppelin/blob/18fa33a9/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java
----------------------------------------------------------------------
diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java
index c2c0338..23a4b1b 100644
--- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java
+++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java
@@ -28,6 +28,7 @@ import java.io.File;
import java.io.IOException;
import java.util.*;
+import com.google.common.collect.Sets;
import org.apache.commons.io.FileUtils;
import org.apache.zeppelin.conf.ZeppelinConfiguration;
import org.apache.zeppelin.conf.ZeppelinConfiguration.ConfVars;
@@ -485,6 +486,13 @@ public class NotebookTest implements JobListenerFactory{
assertEquals(notebookAuthorization.isWriter(note.id(),
new HashSet<String>(Arrays.asList("user1"))), true);
+ // Test clearing of permssions
+ notebookAuthorization.setReaders(note.id(), Sets.<String>newHashSet());
+ assertEquals(notebookAuthorization.isReader(note.id(),
+ new HashSet<String>(Arrays.asList("user2"))), true);
+ assertEquals(notebookAuthorization.isReader(note.id(),
+ new HashSet<String>(Arrays.asList("user3"))), true);
+
notebook.removeNote(note.id());
}