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 <[email protected]> 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 <[email protected]> Authored: Tue Apr 26 12:23:18 2016 -0700 Committer: Lee moon soo <[email protected]> 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()); }
