You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by an...@apache.org on 2012/08/15 15:32:20 UTC

svn commit: r1373396 - in /jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/type/ oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/ oak-core/src/mai...

Author: angela
Date: Wed Aug 15 13:32:19 2012
New Revision: 1373396

URL: http://svn.apache.org/viewvc?rev=1373396&view=rev
Log:
OAK-64 : Privilege Management (WIP)

- make privilege tree mandatory as it is repo-level content similar to node types and versions
- create privilege tree upon repo creation including setting node types
- privilege registration

OAK-66 : NodeType Management

- add FIXME comment to NodeTypeManagerImpl: built-in types should be installed by repository setup as
   the corresponding system node is mandatory and protected and an editing session may not have sufficient
   privileges to write.
- add FIXME regarding migration of custom node types

Added:
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeMigrator.java
Modified:
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ContentRepositoryImpl.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/type/NodeTypeManagerImpl.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeDefinitionImpl.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeDefinitionReader.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeRegistry.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/privilege/PrivilegeDefinition.java
    jackrabbit/oak/trunk/oak-core/src/main/resources/org/apache/jackrabbit/oak/plugins/type/builtin_nodetypes.cnd
    jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/security/privilege/PrivilegeManagerImpl.java

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ContentRepositoryImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ContentRepositoryImpl.java?rev=1373396&r1=1373395&r2=1373396&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ContentRepositoryImpl.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ContentRepositoryImpl.java Wed Aug 15 13:32:19 2012
@@ -40,6 +40,7 @@ import org.apache.jackrabbit.oak.spi.com
 import org.apache.jackrabbit.oak.spi.commit.ValidatorProvider;
 import org.apache.jackrabbit.oak.spi.security.authentication.LoginContextProvider;
 import org.apache.jackrabbit.oak.spi.security.authorization.AccessControlContext;
+import org.apache.jackrabbit.oak.spi.state.NodeState;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -114,13 +115,20 @@ public class ContentRepositoryImpl imple
         // TODO: use configurable context provider
         loginContextProvider = new LoginContextProviderImpl(this);
 
+        // FIXME: repository setup must be done elsewhere...
         // FIXME: depends on CoreValue's name mangling
-        String ntUnstructured = "nam:nt:unstructured";
-
-        // FIXME: workspace setup must be done elsewhere...
-        microKernel.commit("/",
-                "^\"jcr:primaryType\":\"" + ntUnstructured + "\" ",
-                null, null);
+        NodeState root = nodeStore.getRoot();
+        if (root.hasChildNode("jcr:system")) {
+            microKernel.commit("/", "^\"jcr:primaryType\":\"nam:rep:root\" ", null, null);
+        } else {
+            microKernel.commit("/", "^\"jcr:primaryType\":\"nam:rep:root\"" +
+                "+\"jcr:system\":{" +
+                    "\"jcr:primaryType\"    :\"nam:rep:system\"," +
+                    "\"jcr:versionStorage\" :{\"jcr:primaryType\":\"nam:rep:versionStorage\"}," +
+                    "\"jcr:nodeTypes\"      :{\"jcr:primaryType\":\"nam:rep:nodeTypes\"}," +
+                    "\"jcr:activities\"     :{\"jcr:primaryType\":\"nam:rep:Activities\"}," +
+                    "\"rep:privileges\"     :{\"jcr:primaryType\":\"nam:rep:Privileges\"}}", null, null);
+        }
     }
 
     private static QueryIndexProvider getDefaultIndexProvider(MicroKernel mk) {

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/type/NodeTypeManagerImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/type/NodeTypeManagerImpl.java?rev=1373396&r1=1373395&r2=1373396&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/type/NodeTypeManagerImpl.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/type/NodeTypeManagerImpl.java Wed Aug 15 13:32:19 2012
@@ -68,10 +68,14 @@ public class NodeTypeManagerImpl impleme
         this.mapper = mapper;
         this.factory = factory;
 
-        if (session.getCurrentRoot().getTree(NODE_TYPES_PATH) == null) {
+        // FIXME: migrate custom node types as well.
+        // FIXME: registration of built-in node types should be moved to repo-setup
+        //        as the jcr:nodetypes tree is protected and the editing session may
+        //        not have sufficient permission to register node types or may
+        //        even have limited read-permission on the jcr:nodetypes path.
+        if (!nodeTypesInContent()) {
             try {
-                InputStream stream = NodeTypeManagerImpl.class.getResourceAsStream(
-                        "builtin_nodetypes.cnd");
+                InputStream stream = NodeTypeManagerImpl.class.getResourceAsStream("builtin_nodetypes.cnd");
                 try {
                     CompactNodeTypeDefReader<NodeTypeTemplate, Map<String, String>> reader =
                             new CompactNodeTypeDefReader<NodeTypeTemplate, Map<String, String>>(
@@ -356,15 +360,21 @@ public class NodeTypeManagerImpl impleme
     private Tree getOrCreateNodeTypes(Root root) {
         Tree types = root.getTree(NODE_TYPES_PATH);
         if (types == null) {
-            Tree system = root.getTree("/jcr:system");  // FIXME: OAK-221
+            Tree system = root.getTree(JCR_SYSTEM);
             if (system == null) {
-                system = root.getTree("/").addChild(JCR_SYSTEM);
+                system = root.getTree("").addChild(JCR_SYSTEM);
             }
             types = system.addChild(JCR_NODE_TYPES);
         }
         return types;
     }
 
+    private boolean nodeTypesInContent() {
+        Root currentRoot = session.getCurrentRoot();
+        Tree types = currentRoot.getTree(NODE_TYPES_PATH);
+        return types != null && types.getChildrenCount() > 0;
+    }
+
     @Override
     public void unregisterNodeType(String name) throws RepositoryException {
         Tree type = null;

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeDefinitionImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeDefinitionImpl.java?rev=1373396&r1=1373395&r2=1373396&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeDefinitionImpl.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeDefinitionImpl.java Wed Aug 15 13:32:19 2012
@@ -57,8 +57,8 @@ class PrivilegeDefinitionImpl implements
 
     @Nonnull
     @Override
-    public String[] getDeclaredAggregateNames() {
-        return declaredAggregateNames.toArray(new String[declaredAggregateNames.size()]);
+    public Set<String> getDeclaredAggregateNames() {
+        return declaredAggregateNames;
     }
 
     //-------------------------------------------------------------< Object >---

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeDefinitionReader.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeDefinitionReader.java?rev=1373396&r1=1373395&r2=1373396&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeDefinitionReader.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeDefinitionReader.java Wed Aug 15 13:32:19 2012
@@ -94,7 +94,7 @@ class PrivilegeDefinitionReader {
      * @throws RepositoryException
      * @throws IOException
      */
-    static Map<String, PrivilegeDefinition> readCustomDefinitons(InputStream customPrivileges,
+    static PrivilegeDefinition[] readCustomDefinitons(InputStream customPrivileges,
                                                                  NamespaceRegistry nsRegistry) throws RepositoryException, IOException {
         Map<String, PrivilegeDefinition> definitions = new LinkedHashMap<String, PrivilegeDefinition>();
         InputSource src = new InputSource(customPrivileges);
@@ -105,7 +105,7 @@ class PrivilegeDefinitionReader {
             }
             definitions.put(privName, def);
         }
-        return definitions;
+        return definitions.values().toArray(new PrivilegeDefinition[definitions.size()]);
     }
 
 

Added: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeMigrator.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeMigrator.java?rev=1373396&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeMigrator.java (added)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeMigrator.java Wed Aug 15 13:32:19 2012
@@ -0,0 +1,69 @@
+/*
+ * 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.jackrabbit.oak.security.privilege;
+
+import java.io.IOException;
+import java.io.InputStream;
+import javax.jcr.NamespaceRegistry;
+import javax.jcr.RepositoryException;
+
+import org.apache.jackrabbit.oak.api.ContentSession;
+import org.apache.jackrabbit.oak.plugins.name.NamespaceRegistryImpl;
+import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeDefinition;
+
+/**
+ * PrivilegeMigrator is a utility to migrate custom privilege definitions from
+ * a jackrabbit 2 project to oak.
+ */
+public class PrivilegeMigrator {
+
+    private final ContentSession contentSession;
+
+    public PrivilegeMigrator(ContentSession contentSession) {
+        this.contentSession = contentSession;
+    }
+
+    /**
+     *
+     * @throws RepositoryException
+     */
+    public void migrateCustomPrivileges() throws RepositoryException {
+        PrivilegeRegistry pr = new PrivilegeRegistry(contentSession);
+        InputStream stream = null;
+        // TODO: order custom privileges such that validation succeeds.
+        // FIXME: user proper path to jr2 custom privileges stored in fs
+        // jr2 used to be:
+        // new FileSystemResource(fs, "/privileges/custom_privileges.xml").getInputStream()
+        if (stream != null) {
+            try {
+                NamespaceRegistry nsRegistry = new NamespaceRegistryImpl(contentSession);
+                PrivilegeDefinition[] custom = PrivilegeDefinitionReader.readCustomDefinitons(stream, nsRegistry);
+                for (PrivilegeDefinition def : custom) {
+                    pr.registerDefinition(def);
+                }
+            } catch (IOException e) {
+                throw new RepositoryException(e);
+            } finally {
+                try {
+                    stream.close();
+                } catch (IOException e) {
+                    // ignore.
+                }
+            }
+        }
+    }
+}
\ No newline at end of file

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeRegistry.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeRegistry.java?rev=1373396&r1=1373395&r2=1373396&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeRegistry.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeRegistry.java Wed Aug 15 13:32:19 2012
@@ -16,15 +16,10 @@
  */
 package org.apache.jackrabbit.oak.security.privilege;
 
-import java.io.IOException;
-import java.io.InputStream;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.Callable;
 import javax.annotation.Nonnull;
-import javax.jcr.NamespaceRegistry;
 import javax.jcr.RepositoryException;
 
 import org.apache.jackrabbit.JcrConstants;
@@ -37,17 +32,21 @@ import org.apache.jackrabbit.oak.api.Tre
 import org.apache.jackrabbit.oak.core.DefaultConflictHandler;
 import org.apache.jackrabbit.oak.core.ReadOnlyTree;
 import org.apache.jackrabbit.oak.plugins.name.NamespaceConstants;
-import org.apache.jackrabbit.oak.plugins.name.NamespaceRegistryImpl;
 import org.apache.jackrabbit.oak.spi.commit.Validator;
 import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeDefinition;
 import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeProvider;
 import org.apache.jackrabbit.oak.spi.state.NodeState;
 import org.apache.jackrabbit.oak.util.NodeUtil;
-import org.apache.jackrabbit.oak.util.TODO;
 import org.apache.jackrabbit.util.Text;
 
 /**
- * PrivilegeProviderImpl... TODO
+ * PrivilegeRegistry... TODO
+ *
+ *
+ * TODO: define if/how built-in privileges are reflected in the mk
+ * TODO: define if custom privileges are read with editing content session (thus enforcing read permissions)
+ *
+ * FIXME: Session#refresh should refresh privileges exposed
  */
 public class PrivilegeRegistry implements PrivilegeProvider, PrivilegeConstants, Validator {
 
@@ -77,11 +76,6 @@ public class PrivilegeRegistry implement
     public PrivilegeRegistry(ContentSession contentSession) throws RepositoryException {
 
         this.contentSession = contentSession;
-        this.reader = new PrivilegeDefinitionReader(contentSession);
-
-        // TODO: define if/how built-in privileges are reflected in the mk
-        // TODO: define where custom privileges are being stored.
-        // TODO: define if custom privileges are read with editing content session (thus enforcing read permissions)
 
         for (String privilegeName : SIMPLE_PRIVILEGES) {
             PrivilegeDefinition def = new PrivilegeDefinitionImpl(privilegeName, false);
@@ -93,29 +87,8 @@ public class PrivilegeRegistry implement
             definitions.put(privilegeName, def);
         }
 
-        CoreValueFactory vf = contentSession.getCoreValueFactory();
-        Root root = contentSession.getCurrentRoot();
-
-        Tree privilegesTree = root.getTree(PRIVILEGES_PATH);
-        if (privilegesTree == null) {
-            // backwards compatibility: read privileges from file system and update
-            // the content tree.
-            try {
-                NodeUtil system = new NodeUtil(root.getTree(JcrConstants.JCR_SYSTEM), contentSession);
-                NodeUtil privNode = system.addChild(REP_PRIVILEGES, NT_REP_PRIVILEGES);
-
-                migrateCustomPrivileges(privNode);
-
-                root.commit(DefaultConflictHandler.OURS);
-            } catch (IOException e) {
-                throw new RepositoryException(e);
-            } catch (CommitFailedException e) {
-                throw new RepositoryException(e);
-            }
-        }
-
+        this.reader = new PrivilegeDefinitionReader(contentSession);
         definitions.putAll(reader.readDefinitions());
-
         updateJcrAllPrivilege();
     }
 
@@ -136,21 +109,20 @@ public class PrivilegeRegistry implement
             final String privilegeName, final boolean isAbstract,
             final Set<String> declaredAggregateNames)
             throws RepositoryException {
-        // TODO: add proper implementation including
-        // - permission check (possibly delegate to a commit validator),
-        // - validate definition (possibly delegate to commit validator)
-        // - persist the custom definition
-        // - recalculate jcr:all privilege
-        return TODO.dummyImplementation().call(new Callable<PrivilegeDefinition>() {
-            @Override
-            public PrivilegeDefinition call() throws Exception {
-                PrivilegeDefinition definition = new PrivilegeDefinitionImpl(
-                        privilegeName, isAbstract,
-                        new HashSet<String>(declaredAggregateNames));
-                definitions.put(privilegeName, definition);
-                return definition;
-            }
-        });
+
+        PrivilegeDefinition definition = new PrivilegeDefinitionImpl(privilegeName, isAbstract, declaredAggregateNames);
+        internalRegisterDefinitions(definition);
+        return definition;
+    }
+
+    public void registerDefinition(PrivilegeDefinition definition) throws RepositoryException {
+        PrivilegeDefinition toRegister;
+        if (definition instanceof PrivilegeDefinitionImpl) {
+            toRegister = definition;
+        } else {
+            toRegister = new PrivilegeDefinitionImpl(definition.getName(), definition.isAbstract(), definition.getDeclaredAggregateNames());
+        }
+        internalRegisterDefinitions(toRegister);
     }
 
     //----------------------------------------------------------< Validator >---
@@ -172,6 +144,7 @@ public class PrivilegeRegistry implement
     @Override
     public Validator childNodeAdded(String name, NodeState after) throws CommitFailedException {
         // the following characteristics are expected to be validated elsewhere:
+        // - permission to allow privilege registration -> permission validator.
         // - name collisions (-> delegated to NodeTypeValidator since sms are not allowed)
         // - name must be valid (-> delegated to NameValidator)
 
@@ -188,7 +161,7 @@ public class PrivilegeRegistry implement
             throw new CommitFailedException("Privilege definition must have primary node type set to rep:privilege");
         }
 
-        // additional validation of the definition include:
+        // additional validation of the definition
         PrivilegeDefinition def = reader.readDefinition(tree);
         validateDefinition(def);
 
@@ -208,22 +181,6 @@ public class PrivilegeRegistry implement
 
     //------------------------------------------------------------< private >---
 
-    private void migrateCustomPrivileges(NodeUtil privilegesNode) throws RepositoryException, IOException, CommitFailedException {
-        InputStream stream = null;
-        // TODO: user proper path to jr2 custom privileges stored in fs
-        // jr2 used to be:
-        // new FileSystemResource(fs, "/privileges/custom_privileges.xml").getInputStream()
-        if (stream != null) {
-            try {
-                NamespaceRegistry  nsRegistry = new NamespaceRegistryImpl(contentSession);
-                Map<String, PrivilegeDefinition> custom = PrivilegeDefinitionReader.readCustomDefinitons(stream, nsRegistry);
-                writeDefinitions(privilegesNode, custom);
-            } finally {
-                stream.close();
-            }
-        }
-    }
-
     private void updateJcrAllPrivilege() {
         // TODO: add proper implementation taking custom privileges into account.
         definitions.put(JCR_ALL, new PrivilegeDefinitionImpl(JCR_ALL, false,
@@ -233,28 +190,100 @@ public class PrivilegeRegistry implement
                 JCR_NAMESPACE_MANAGEMENT, REP_PRIVILEGE_MANAGEMENT, REP_WRITE));
     }
 
-    private void writeDefinitions(NodeUtil privilegesNode, Map<String, PrivilegeDefinition> definitions) {
-        for (PrivilegeDefinition def : definitions.values()) {
-            NodeUtil privNode = privilegesNode.addChild(def.getName(), NT_REP_PRIVILEGE);
-            if (def.isAbstract()) {
-                privNode.setBoolean(REP_IS_ABSTRACT, true);
-            }
-            String[] declAggrNames = def.getDeclaredAggregateNames();
-            if (declAggrNames.length > 0) {
-                privNode.setNames(REP_AGGREGATES, def.getDeclaredAggregateNames());
+    private void internalRegisterDefinitions(PrivilegeDefinition toRegister) throws RepositoryException {
+        CoreValueFactory vf = contentSession.getCoreValueFactory();
+        Root root = contentSession.getCurrentRoot();
+
+        try {
+            // make sure the privileges path is defined
+            Tree privilegesTree = root.getTree(PRIVILEGES_PATH);
+            if (privilegesTree == null) {
+                throw new RepositoryException("Repository doesn't contain node " + PRIVILEGES_PATH);
             }
+
+            NodeUtil privilegesNode = new NodeUtil(privilegesTree, contentSession);
+            writeDefinition(privilegesNode, toRegister);
+
+            // delegate validation to the commit validation (see above)
+            root.commit(DefaultConflictHandler.OURS);
+
+        } catch (CommitFailedException e) {
+            throw new RepositoryException(e.getMessage());
+        }
+
+        definitions.put(toRegister.getName(), toRegister);
+        updateJcrAllPrivilege();
+    }
+
+    private void writeDefinition(NodeUtil privilegesNode, PrivilegeDefinition definition) {
+        NodeUtil privNode = privilegesNode.addChild(definition.getName(), NT_REP_PRIVILEGE);
+        if (definition.isAbstract()) {
+            privNode.setBoolean(REP_IS_ABSTRACT, true);
+        }
+        Set<String> declAggrNames = definition.getDeclaredAggregateNames();
+        if (!declAggrNames.isEmpty()) {
+            String[] names = definition.getDeclaredAggregateNames().toArray(new String[declAggrNames.size()]);
+            privNode.setNames(REP_AGGREGATES, names);
         }
     }
 
     /**
+     * Validation of the privilege definition including the following steps:
+     *
+     * - all aggregates must have been registered before
+     * - no existing privilege defines the same aggregation
+     * - no cyclic aggregation
      *
-     * @param definition
+     * @param definition The new privilege definition to validate.
+     * @throws org.apache.jackrabbit.oak.api.CommitFailedException If any of
+     * the checks listed above fails.
      */
-    private void validateDefinition(PrivilegeDefinition definition) {
-        // TODO
-        // - aggregate names refer to existing privileges
-        // - aggregate names do not create cyclic dependencies
-        // - aggregate names are not covered by an existing privilege definition
-        // -
+    private void validateDefinition(PrivilegeDefinition definition) throws CommitFailedException {
+        Set<String> aggrNames = definition.getDeclaredAggregateNames();
+        if (aggrNames.isEmpty()) {
+            return;
+        }
+
+        for (String aggrName : aggrNames) {
+            // aggregated privilege not registered
+            if (!definitions.containsKey(aggrName)) {
+                throw new CommitFailedException("Declared aggregate '"+ aggrName +"' is not a registered privilege.");
+            }
+
+            // check for circular aggregation
+            if (isCircularAggregation(definition.getName(), aggrName)) {
+                String msg = "Detected circular aggregation within custom privilege caused by " + aggrName;
+                throw new CommitFailedException(msg);
+            }
+        }
+
+        for (PrivilegeDefinition existing : definitions.values()) {
+            if (aggrNames.equals(existing.getDeclaredAggregateNames())) {
+                String msg = "Custom aggregate privilege '" + definition.getName() + "' is already covered by '" + existing.getName() + '\'';
+                throw new CommitFailedException(msg);
+            }
+        }
+    }
+
+    private boolean isCircularAggregation(String privilegeName, String aggregateName) {
+        if (privilegeName.equals(aggregateName)) {
+            return true;
+        }
+
+        PrivilegeDefinition aggrPriv = definitions.get(aggregateName);
+        if (aggrPriv.getDeclaredAggregateNames().isEmpty()) {
+            return false;
+        } else {
+            boolean isCircular = false;
+            for (String name : aggrPriv.getDeclaredAggregateNames()) {
+                if (privilegeName.equals(name)) {
+                    return true;
+                }
+                if (definitions.containsKey(name)) {
+                    isCircular = isCircularAggregation(privilegeName, name);
+                }
+            }
+            return isCircular;
+        }
     }
 }
\ No newline at end of file

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/privilege/PrivilegeDefinition.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/privilege/PrivilegeDefinition.java?rev=1373396&r1=1373395&r2=1373396&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/privilege/PrivilegeDefinition.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/security/privilege/PrivilegeDefinition.java Wed Aug 15 13:32:19 2012
@@ -16,6 +16,7 @@
  */
 package org.apache.jackrabbit.oak.spi.security.privilege;
 
+import java.util.Set;
 import javax.annotation.Nonnull;
 
 /**
@@ -49,5 +50,5 @@ public interface PrivilegeDefinition {
      * @return The internal names of the aggregated privileges or an empty array.
      */
     @Nonnull
-    String[] getDeclaredAggregateNames();
+    Set<String> getDeclaredAggregateNames();
 }
\ No newline at end of file

Modified: jackrabbit/oak/trunk/oak-core/src/main/resources/org/apache/jackrabbit/oak/plugins/type/builtin_nodetypes.cnd
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/resources/org/apache/jackrabbit/oak/plugins/type/builtin_nodetypes.cnd?rev=1373396&r1=1373395&r2=1373396&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/resources/org/apache/jackrabbit/oak/plugins/type/builtin_nodetypes.cnd (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/resources/org/apache/jackrabbit/oak/plugins/type/builtin_nodetypes.cnd Wed Aug 15 13:32:19 2012
@@ -503,7 +503,7 @@
   + jcr:configurations (rep:Configurations) = rep:Configurations protected ABORT
   + * (nt:base) = nt:base IGNORE
   // @since oak 1.0
-  + rep:privileges (rep:Privileges) = rep:Privileges protected ABORT
+  + rep:privileges (rep:Privileges) = rep:Privileges mandatory protected ABORT
 
 
 /**

Modified: jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/security/privilege/PrivilegeManagerImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/security/privilege/PrivilegeManagerImpl.java?rev=1373396&r1=1373395&r2=1373396&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/security/privilege/PrivilegeManagerImpl.java (original)
+++ jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/security/privilege/PrivilegeManagerImpl.java Wed Aug 15 13:32:19 2012
@@ -119,13 +119,13 @@ public class PrivilegeManagerImpl implem
 
         @Override
         public boolean isAggregate() {
-            return definition.getDeclaredAggregateNames().length > 0;
+            return !definition.getDeclaredAggregateNames().isEmpty();
         }
 
         @Override
         public Privilege[] getDeclaredAggregatePrivileges() {
-            String[] declaredAggregateNames = definition.getDeclaredAggregateNames();
-            Set<Privilege> declaredAggregates = new HashSet<Privilege>(declaredAggregateNames.length);
+            Set<String> declaredAggregateNames = definition.getDeclaredAggregateNames();
+            Set<Privilege> declaredAggregates = new HashSet<Privilege>(declaredAggregateNames.size());
             for (String pName : declaredAggregateNames) {
                 try {
                     declaredAggregates.add(getPrivilege(pName));