OAK-10334: Node.addMixin() may overwrite existing mixins

Allow adding a mixin type even if session does not have permission to read jcr:mixinTypes. This is consistent with behaviour of Node.getMixinNodeTypes(). See also OAK-2441.

oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/NodeDelegate.java

@@ -389,8 +389,8 @@ public boolean canAddMixin(String typeName) throws RepositoryException {
         }
     }
 
-    public void addMixin(String typeName) throws RepositoryException {
-        TreeUtil.addMixin(getTree(), typeName, sessionDelegate.getRoot().getTree(NODE_TYPES_PATH), getUserID());
+    public void addMixin(Function<Tree, Iterable<String>> existingMixins, String typeName) throws RepositoryException {
+        TreeUtil.addMixin(getTree(), existingMixins, typeName, sessionDelegate.getRoot().getTree(NODE_TYPES_PATH), getUserID());
     }
 
     public void removeMixin(String typeName) throws RepositoryException {
@@ -421,7 +421,7 @@ public void setMixins(Set<String> mixinNames) throws RepositoryException {
     public void updateMixins(Set<String> addMixinNames, Set<String> removedOakMixinNames) throws RepositoryException {
         // 1. set all new mixin types including validation
         for (String oakMixinName : addMixinNames) {
-            addMixin(oakMixinName);
+            addMixin(TreeUtil::getMixinTypeNames, oakMixinName);
         }
 
         if (!removedOakMixinNames.isEmpty()) {

oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/session/NodeImpl.java

@@ -953,7 +953,7 @@ public NodeType[] getMixinNodeTypes() throws RepositoryException {
             public NodeType[] perform() throws RepositoryException {
                 Tree tree = node.getTree();
 
-                Iterator<String> mixinNames = getMixinTypeNames(tree);
+                Iterator<String> mixinNames = getMixinTypeNames(tree).iterator();
                 if (mixinNames.hasNext()) {
                     NodeTypeManager ntMgr = getNodeTypeManager();
                     List<NodeType> mixinTypes = Lists.newArrayList();
@@ -976,8 +976,7 @@ public boolean isNodeType(String nodeTypeName) throws RepositoryException {
             @Override
             public Boolean perform() throws RepositoryException {
                 Tree tree = node.getTree();
-                Iterable<String> mixins = () -> getMixinTypeNames(tree);
-                return getNodeTypeManager().isNodeType(getPrimaryTypeName(tree), mixins, oakName);
+                return getNodeTypeManager().isNodeType(getPrimaryTypeName(tree), getMixinTypeNames(tree), oakName);
             }
         });
     }
@@ -1015,13 +1014,10 @@ public void checkPreconditions() throws RepositoryException {
                     throw new VersionException(format(
                             "Cannot add mixin type. Node [%s] is checked in.", getNodePath()));
                 }
-                // OAK-10334: adding mixin requires permission to read existing mixin types
-                PropertyState prop = PropertyStates.createProperty(JCR_MIXINTYPES, singleton(oakTypeName), NAMES);
-                sessionContext.getAccessManager().checkPermissions(dlg.getTree(), prop, Permissions.READ_PROPERTY);
             }
             @Override
             public void performVoid() throws RepositoryException {
-                dlg.addMixin(oakTypeName);
+                dlg.addMixin(NodeImpl.this::getMixinTypeNames, oakTypeName);
             }
         });
     }
@@ -1349,11 +1345,11 @@ private String getPrimaryTypeName(@NotNull Tree tree) {
     }
 
     @NotNull
-    private Iterator<String> getMixinTypeNames(@NotNull Tree tree) {
+    private Iterable<String> getMixinTypeNames(@NotNull Tree tree) {
         if (tree.hasProperty(JcrConstants.JCR_MIXINTYPES) || canReadMixinTypes(tree)) {
-            return TreeUtil.getMixinTypeNames(tree).iterator();
+            return TreeUtil.getMixinTypeNames(tree);
         } else {
-            return TreeUtil.getMixinTypeNames(tree, getReadOnlyTree(tree)).iterator();
+            return TreeUtil.getMixinTypeNames(tree, getReadOnlyTree(tree));
         }
     }
 

oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/ReadPropertyTest.java

@@ -204,18 +204,17 @@ public void testAddMixinType() throws Exception {
         assertMixinTypes(superuser.getNode(path), MIX_REFERENCEABLE, MIX_REP_ACCESS_CONTROLLABLE);
 
         Node node = testSession.getNode(path);
+        String mixTitle = "mix:title";
         assertFalse(node.hasProperty(JcrConstants.JCR_MIXINTYPES));
-        try {
-            node.addMixin("mix:title");
-        } catch (AccessDeniedException e) {
-            // permitted to fail because reading jcr:mixinTypes was denied
-            return;
-        }
-        // if we get here, then node.addMixin() was successful
+        assertTrue(node.canAddMixin(mixTitle));
+
+        // must be able to add mixin even if session
+        // does not have permission to read jcr:mixinTypes
+        node.addMixin(mixTitle);
         testSession.save();
-        // then we should be able to see all three mixin types
-        superuser.refresh(false);
-        assertMixinTypes(superuser.getNode(path), MIX_REFERENCEABLE, MIX_REP_ACCESS_CONTROLLABLE, "mix:title");
+
+        // we should be able to see all three mixin types
+        assertMixinTypes(superuser.getNode(path), MIX_REFERENCEABLE, MIX_REP_ACCESS_CONTROLLABLE, mixTitle);
     }
 
     private void assertMixinTypes(Node node, String... mixins)

oak-security-spi/src/main/java/org/apache/jackrabbit/oak/plugins/tree/TreeUtil.java

@@ -19,6 +19,7 @@
 import java.util.Calendar;
 import java.util.List;
 import java.util.Set;
+import java.util.function.Function;
 
 import javax.jcr.AccessDeniedException;
 import javax.jcr.RepositoryException;
@@ -310,7 +311,47 @@ public static Tree getOrAddChild(@NotNull Tree tree, @NotNull String childName,
         return (child.exists()) ? child : addChild(tree, childName, primaryTypeName);
     }
 
+    /**
+     * Add a mixin type to the given {@code tree}. The implementation checks
+     * the effective type of the tree and will not add the mixin if it
+     * determines the tree is already of type {@code mixinName} through the
+     * currently set primary or mixin types, directly or indirectly by type
+     * inheritance.
+     *
+     * @param tree tree where the mixin type is to be added.
+     * @param mixinName name of the mixin to add.
+     * @param typeRoot tree where type information is stored.
+     * @param userID user id or {@code null} if unknown.
+     * @throws RepositoryException if {@code mixinName} does not refer to an
+     *      existing type or the type it refers to is abstract or the type it
+     *      refers to is a primary type.
+     */
     public static void addMixin(@NotNull Tree tree, @NotNull String mixinName, @NotNull Tree typeRoot, @Nullable String userID) throws RepositoryException {
+        addMixin(tree, t -> getNames(t, JCR_MIXINTYPES), mixinName, typeRoot, userID);
+    }
+
+    /**
+     * Add a mixin type to the given {@code tree}. The implementation checks
+     * the effective type of the tree and will not add the mixin if it
+     * determines the tree is already of type {@code mixinName} through the
+     * currently set primary or mixin types, directly or indirectly by type
+     * inheritance.
+     *
+     * @param tree tree where the mixin type is to be added.
+     * @param existingMixins function to get the currently set mixin types from
+     *      a tree.
+     * @param mixinName name of the mixin to add.
+     * @param typeRoot tree where type information is stored.
+     * @param userID user id or {@code null} if unknown.
+     * @throws RepositoryException if {@code mixinName} does not refer to an
+     *      existing type or the type it refers to is abstract or the type it
+     *      refers to is a primary type.
+     */
+    public static void addMixin(@NotNull Tree tree,
+                                @NotNull Function<Tree, Iterable<String>> existingMixins,
+                                @NotNull String mixinName,
+                                @NotNull Tree typeRoot,
+                                @Nullable String userID) throws RepositoryException {
         Tree type = typeRoot.getChild(mixinName);
         if (!type.exists()) {
             throw noSuchNodeTypeException(mixinName);
@@ -327,7 +368,7 @@ public static void addMixin(@NotNull Tree tree, @NotNull String mixinName, @NotN
         }
 
         Set<String> subMixins = Sets.newHashSet(getNames(type, NodeTypeConstants.REP_MIXIN_SUBTYPES));
-        for (String mixin : getNames(tree, JCR_MIXINTYPES)) {
+        for (String mixin : existingMixins.apply(tree)) {
             if (mixinName.equals(mixin) || subMixins.contains(mixin)) {
                 return;
             }

oak-security-spi/src/main/java/org/apache/jackrabbit/oak/plugins/tree/package-info.java

@@ -14,7 +14,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-@Version("3.3.0")
+@Version("3.4.0")
 package org.apache.jackrabbit.oak.plugins.tree;
 
 import org.osgi.annotation.versioning.Version;