ZOOKEEPER-1055. check for duplicate ACLs in addACL() and create(). (Eugene Koontz via mahadev)

git-svn-id: https://svn.apache.org/repos/asf/zookeeper/trunk@1157685 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -285,6 +285,9 @@ BUGFIXES:
   ZOOKEEPER-1150. fix for this patch to compile on windows. (dheeraj
   via mahadev)
 
+  ZOOKEEPER-1055. check for duplicate ACLs in addACL() and create().
+  (Eugene Koontz via mahadev)
+
 IMPROVEMENTS:
   ZOOKEEPER-724. Improve junit test integration - log harness information 
   (phunt via mahadev)

src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java

@@ -305,7 +305,8 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                             Long.toHexString(request.sessionId));
                     throw new KeeperException.BadArgumentsException(path);
                 }
-                if (!fixupACL(request.authInfo, createRequest.getAcl())) {
+                List<ACL> listACL = removeDuplicates(createRequest.getAcl());
+                if (!fixupACL(request.authInfo, listACL)) {
                     throw new KeeperException.InvalidACLException(path);
                 }
                 String parentPath = path.substring(0, lastSlash);
@@ -339,7 +340,7 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                 }
                 int newCversion = parentRecord.stat.getCversion()+1;
                 request.txn = new CreateTxn(path, createRequest.getData(),
-                        createRequest.getAcl(),
+                        listACL,
                         createMode.isEphemeral(), newCversion);
                 StatPersisted s = new StatPersisted();
                 if (createMode.isEphemeral()) {
@@ -350,8 +351,7 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                 parentRecord.stat.setCversion(newCversion);
                 addChangeRecord(parentRecord);
                 addChangeRecord(new ChangeRecord(request.hdr.getZxid(), path, s,
-                        0, createRequest.getAcl()));
-
+                        0, listACL));
                 break;
             case OpCode.delete:
                 zks.sessionTracker.checkSession(request.sessionId, request.getOwner());
@@ -403,7 +403,8 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                 zks.sessionTracker.checkSession(request.sessionId, request.getOwner());
                 SetACLRequest setAclRequest = (SetACLRequest)record;
                 path = setAclRequest.getPath();
-                if (!fixupACL(request.authInfo, setAclRequest.getAcl())) {
+                listACL = removeDuplicates(setAclRequest.getAcl());
+                if (!fixupACL(request.authInfo, listACL)) {
                     throw new KeeperException.InvalidACLException(path);
                 }
                 nodeRecord = getRecordForPath(path);
@@ -415,7 +416,7 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                     throw new KeeperException.BadVersionException(path);
                 }
                 version = currentVersion + 1;
-                request.txn = new SetACLTxn(path, setAclRequest.getAcl(), version);
+                request.txn = new SetACLTxn(path, listACL, version);
                 nodeRecord = nodeRecord.duplicate(request.hdr.getZxid());
                 nodeRecord.stat.setAversion(version);
                 addChangeRecord(nodeRecord);
@@ -629,6 +630,20 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
         nextProcessor.processRequest(request);
     }
 
+    private List<ACL> removeDuplicates(List<ACL> acl) {
+
+        ArrayList<ACL> retval = new ArrayList<ACL>();
+        Iterator<ACL> it = acl.iterator();
+        while (it.hasNext()) {
+            ACL a = it.next();
+            if (retval.contains(a) == false) {
+                retval.add(a);
+            }
+        }
+        return retval;
+    }
+
+
     /**
      * This method checks out the acl making sure it isn't null or empty,
      * it has valid schemes and ids, and expanding any relative ids that
@@ -645,6 +660,7 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
         if (acl == null || acl.size() == 0) {
             return false;
         }
+
         Iterator<ACL> it = acl.iterator();
         LinkedList<ACL> toAdd = null;
         while (it.hasNext()) {

src/java/main/org/apache/zookeeper/server/ServerCnxn.java

@@ -77,7 +77,9 @@ public abstract class ServerCnxn implements Stats, Watcher {
     }
 
     public void addAuthInfo(Id id) {
-        authInfo.add(id);
+        if (authInfo.contains(id) == false) {
+            authInfo.add(id);
+        }
     }
 
     public boolean removeAuthInfo(Id id) {

src/java/test/org/apache/zookeeper/test/ACLCountTest.java

@@ -0,0 +1,136 @@
+/**
+ * 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.zookeeper.test;
+
+import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.concurrent.CountDownLatch;
+import java.util.List;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.Watcher.Event.KeeperState;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.SyncRequestProcessor;
+import org.apache.zookeeper.server.ZooKeeperServer;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ACLCountTest extends ZKTestCase implements Watcher {
+    private static final Logger LOG = LoggerFactory.getLogger(ACLTest.class);
+    private static final String HOSTPORT =
+        "127.0.0.1:" + PortAssignment.unique();
+    private volatile CountDownLatch startSignal;
+
+    /**
+     *
+     * Create a node and add 4 ACL values to it, but there are only 2 unique ACL values,
+     * and each is repeated once:
+     *
+     *   ACL(ZooDefs.Perms.READ,ZooDefs.Ids.ANYONE_ID_UNSAFE);
+     *   ACL(ZooDefs.Perms.ALL,ZooDefs.Ids.AUTH_IDS);
+     *   ACL(ZooDefs.Perms.READ,ZooDefs.Ids.ANYONE_ID_UNSAFE);
+     *   ACL(ZooDefs.Perms.ALL,ZooDefs.Ids.AUTH_IDS);
+     *
+     * Even though we've added 4 ACL values, there should only be 2 ACLs for that node,
+     * since there are only 2 *unique* ACL values.
+     */
+    @Test
+    public void testAclCount() throws Exception {
+        File tmpDir = ClientBase.createTmpDir();
+        ClientBase.setupTestEnv();
+        ZooKeeperServer zks = new ZooKeeperServer(tmpDir, tmpDir, 3000);
+        SyncRequestProcessor.setSnapCount(1000);
+        final int PORT = Integer.parseInt(HOSTPORT.split(":")[1]);
+        ServerCnxnFactory f = ServerCnxnFactory.createFactory(PORT, -1);
+        f.startup(zks);
+        ZooKeeper zk;
+
+        final ArrayList<ACL> CREATOR_ALL_AND_WORLD_READABLE =
+          new ArrayList<ACL>() { {
+            add(new ACL(ZooDefs.Perms.READ,ZooDefs.Ids.ANYONE_ID_UNSAFE));
+            add(new ACL(ZooDefs.Perms.ALL,ZooDefs.Ids.AUTH_IDS));
+            add(new ACL(ZooDefs.Perms.READ,ZooDefs.Ids.ANYONE_ID_UNSAFE));
+            add(new ACL(ZooDefs.Perms.ALL,ZooDefs.Ids.AUTH_IDS));
+        }};
+
+        try {
+            LOG.info("starting up the zookeeper server .. waiting");
+            Assert.assertTrue("waiting for server being up",
+                    ClientBase.waitForServerUp(HOSTPORT, CONNECTION_TIMEOUT));
+            zk = new ZooKeeper(HOSTPORT, CONNECTION_TIMEOUT, this);
+
+            zk.addAuthInfo("digest", "pat:test".getBytes());
+            zk.setACL("/", Ids.CREATOR_ALL_ACL, -1);
+
+            String path = "/path";
+
+            try {
+              Assert.assertEquals(4,CREATOR_ALL_AND_WORLD_READABLE.size());
+            }
+            catch (Exception e) {
+              LOG.error("Something is fundamentally wrong with ArrayList's add() method. add()ing four times to an empty ArrayList should result in an ArrayList with 4 members.");
+              throw e;
+            }
+
+            zk.create(path,path.getBytes(),CREATOR_ALL_AND_WORLD_READABLE,CreateMode.PERSISTENT);
+            List<ACL> acls = zk.getACL("/path", new Stat());
+            Assert.assertEquals(2,acls.size());
+        }
+        catch (Exception e) {
+          // test failed somehow.
+          Assert.assertTrue(false);
+        }
+
+        Assert.assertTrue(true);
+
+    }
+
+
+    /*
+     * (non-Javadoc)
+     *
+     * @see org.apache.zookeeper.Watcher#process(org.apache.zookeeper.WatcherEvent)
+     */
+    public void process(WatchedEvent event) {
+        LOG.info("Event:" + event.getState() + " " + event.getType() + " "
+                 + event.getPath());
+        if (event.getState() == KeeperState.SyncConnected) {
+            if (startSignal != null && startSignal.getCount() > 0) {
+                LOG.info("startsignal.countDown()");
+                startSignal.countDown();
+            } else {
+                LOG.warn("startsignal " + startSignal);
+            }
+        }
+    }
+
+}