Fix bug with elevating access mode

- Modify javadocs - Replace concept of elevated access mode with overridden access mode
neo4j · Oct 12, 2016 · 2a08c9b · 2a08c9b
1 parent 1b5ac9f
commit 2a08c9b
Show file tree

Hide file tree

Showing 7 changed files with 61 additions and 50 deletions.
diff --git a/...pher/src/main/scala/org/neo4j/cypher/internal/spi/v3_1/TransactionBoundQueryContext.scala b/...pher/src/main/scala/org/neo4j/cypher/internal/spi/v3_1/TransactionBoundQueryContext.scala
@@ -597,7 +597,7 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional
   override def callReadOnlyProcedure(name: QualifiedName, args: Seq[Any], allowed: Array[String]) = {
     val call: KernelProcedureCall =
       if (shouldElevate(allowed))
-        transactionalContext.statement.procedureCallOperations.procedureCallReadElevated(_, _)
+        transactionalContext.statement.procedureCallOperations.procedureCallReadOverride(_, _)
       else
         transactionalContext.statement.procedureCallOperations.procedureCallRead(_, _)
     callProcedure(name, args, call)
@@ -606,7 +606,7 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional
   override def callReadWriteProcedure(name: QualifiedName, args: Seq[Any], allowed: Array[String]) = {
     val call: KernelProcedureCall =
       if (shouldElevate(allowed))
-        transactionalContext.statement.procedureCallOperations.procedureCallWriteElevated(_, _)
+        transactionalContext.statement.procedureCallOperations.procedureCallWriteOverride(_, _)
       else
         transactionalContext.statement.procedureCallOperations.procedureCallWrite(_, _)
     callProcedure(name, args, call)
@@ -615,7 +615,7 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional
   override def callSchemaWriteProcedure(name: QualifiedName, args: Seq[Any], allowed: Array[String]) = {
     val call: KernelProcedureCall =
       if (shouldElevate(allowed))
-        transactionalContext.statement.procedureCallOperations.procedureCallSchemaElevated(_, _)
+        transactionalContext.statement.procedureCallOperations.procedureCallSchemaOverride(_, _)
       else
         transactionalContext.statement.procedureCallOperations.procedureCallSchema(_, _)
     callProcedure(name, args, call)
@@ -638,7 +638,7 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional
   override def callFunction(name: QualifiedName, args: Seq[Any], allowed: Array[String]) = {
     val call: KernelFunctionCall =
       if (shouldElevate(allowed))
-        transactionalContext.statement.procedureCallOperations.functionCallElevated(_, _)
+        transactionalContext.statement.procedureCallOperations.functionCallOverride(_, _)
       else
         transactionalContext.statement.procedureCallOperations.functionCall(_, _)
     callFunction(name, args, call)

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/ProcedureCallOperations.java b/community/kernel/src/main/java/org/neo4j/kernel/api/ProcedureCallOperations.java
@@ -39,14 +39,14 @@ RawIterator<Object[], ProcedureException> procedureCallRead( QualifiedName name,
             throws ProcedureException;
 
     /**
-     * Invoke a read-only procedure by name, and elevate the transaction's access mode to
-     * the given access mode for the duration of the procedure execution.
+     * Invoke a read-only procedure by name, and set the transaction's access mode to
+     * {@link org.neo4j.kernel.api.security.AccessMode.Static#READ READ} for the duration of the procedure execution.
      * @param name the name of the procedure.
      * @param arguments the procedure arguments.
      * @return an iterator containing the procedure results.
      * @throws ProcedureException if there was an exception thrown during procedure execution.
      */
-    RawIterator<Object[], ProcedureException> procedureCallReadElevated( QualifiedName name, Object[] arguments )
+    RawIterator<Object[], ProcedureException> procedureCallReadOverride( QualifiedName name, Object[] arguments )
             throws ProcedureException;
 
     /**
@@ -59,14 +59,14 @@ RawIterator<Object[], ProcedureException> procedureCallReadElevated( QualifiedNa
     RawIterator<Object[], ProcedureException> procedureCallWrite( QualifiedName name, Object[] arguments )
             throws ProcedureException;
     /**
-     * Invoke a read/write procedure by name, and elevate the transaction's access mode to
-     * the given access mode for the duration of the procedure execution.
+     * Invoke a read-only procedure by name, and set the transaction's access mode to
+     * {@link org.neo4j.kernel.api.security.AccessMode.Static#WRITE WRITE} for the duration of the procedure execution.
      * @param name the name of the procedure.
      * @param arguments the procedure arguments.
      * @return an iterator containing the procedure results.
      * @throws ProcedureException if there was an exception thrown during procedure execution.
      */
-    RawIterator<Object[], ProcedureException> procedureCallWriteElevated( QualifiedName name, Object[] arguments )
+    RawIterator<Object[], ProcedureException> procedureCallWriteOverride( QualifiedName name, Object[] arguments )
             throws ProcedureException;
 
     /**
@@ -79,14 +79,14 @@ RawIterator<Object[], ProcedureException> procedureCallWriteElevated( QualifiedN
     RawIterator<Object[], ProcedureException> procedureCallSchema( QualifiedName name, Object[] arguments )
             throws ProcedureException;
     /**
-     * Invoke a schema write procedure by name, and elevate the transaction's access mode to
-     * the given access mode for the duration of the procedure execution.
+     * Invoke a read-only procedure by name, and set the transaction's access mode to
+     * {@link org.neo4j.kernel.api.security.AccessMode.Static#FULL FULL} for the duration of the procedure execution.
      * @param name the name of the procedure.
      * @param arguments the procedure arguments.
      * @return an iterator containing the procedure results.
      * @throws ProcedureException if there was an exception thrown during procedure execution.
      */
-    RawIterator<Object[], ProcedureException> procedureCallSchemaElevated( QualifiedName name, Object[] arguments )
+    RawIterator<Object[], ProcedureException> procedureCallSchemaOverride( QualifiedName name, Object[] arguments )
             throws ProcedureException;
 
     /** Invoke a read-only function by name
@@ -96,11 +96,11 @@ RawIterator<Object[], ProcedureException> procedureCallSchemaElevated( Qualified
      */
     Object functionCall( QualifiedName name, Object[] arguments ) throws ProcedureException;
 
-    /** Invoke a read-only function by name, and elevate the transaction's access mode to
-     * the given access mode for the duration of the function execution.
+    /** Invoke a read-only function by name, and set the transaction's access mode to
+     * {@link org.neo4j.kernel.api.security.AccessMode.Static#READ READ} for the duration of the function execution.
      * @param name the name of the function.
      * @param arguments the function arguments.
      * @throws ProcedureException if there was an exception thrown during function execution.
      */
-    Object functionCallElevated( QualifiedName name, Object[] arguments ) throws ProcedureException;
+    Object functionCallOverride( QualifiedName name, Object[] arguments ) throws ProcedureException;
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/OperationsFacade.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/OperationsFacade.java
@@ -91,7 +91,7 @@
 import org.neo4j.kernel.impl.api.operations.QueryRegistrationOperations;
 import org.neo4j.kernel.impl.api.operations.SchemaReadOperations;
 import org.neo4j.kernel.impl.api.operations.SchemaStateOperations;
-import org.neo4j.kernel.impl.api.security.ElevatedAccessMode;
+import org.neo4j.kernel.impl.api.security.OverriddenAccessMode;
 import org.neo4j.kernel.impl.api.security.RestrictedAccessMode;
 import org.neo4j.kernel.impl.api.store.RelationshipIterator;
 import org.neo4j.kernel.impl.proc.Procedures;
@@ -1502,9 +1502,9 @@ public RawIterator<Object[], ProcedureException> procedureCallRead( QualifiedNam
     }
 
     @Override
-    public RawIterator<Object[], ProcedureException> procedureCallReadElevated( QualifiedName name, Object[] input ) throws ProcedureException
+    public RawIterator<Object[], ProcedureException> procedureCallReadOverride( QualifiedName name, Object[] input ) throws ProcedureException
     {
-        return callProcedure( name, input, new ElevatedAccessMode( tx.mode(), AccessMode.Static.READ ) );
+        return callProcedure( name, input, new OverriddenAccessMode( tx.mode(), AccessMode.Static.READ ) );
     }
 
     @Override
@@ -1518,9 +1518,9 @@ public RawIterator<Object[], ProcedureException> procedureCallWrite( QualifiedNa
     }
 
     @Override
-    public RawIterator<Object[], ProcedureException> procedureCallWriteElevated( QualifiedName name, Object[] input ) throws ProcedureException
+    public RawIterator<Object[], ProcedureException> procedureCallWriteOverride( QualifiedName name, Object[] input ) throws ProcedureException
     {
-        return callProcedure( name, input, new ElevatedAccessMode( tx.mode(), AccessMode.Static.WRITE ) );
+        return callProcedure( name, input, new OverriddenAccessMode( tx.mode(), AccessMode.Static.WRITE ) );
     }
 
     @Override
@@ -1534,9 +1534,9 @@ public RawIterator<Object[], ProcedureException> procedureCallSchema( QualifiedN
     }
 
     @Override
-    public RawIterator<Object[], ProcedureException> procedureCallSchemaElevated( QualifiedName name, Object[] input ) throws ProcedureException
+    public RawIterator<Object[], ProcedureException> procedureCallSchemaOverride( QualifiedName name, Object[] input ) throws ProcedureException
     {
-        return callProcedure( name, input, new ElevatedAccessMode( tx.mode(), AccessMode.Static.FULL ) );
+        return callProcedure( name, input, new OverriddenAccessMode( tx.mode(), AccessMode.Static.FULL ) );
     }
 
     private RawIterator<Object[],ProcedureException> callProcedure(
@@ -1586,9 +1586,9 @@ public Object functionCall( QualifiedName name, Object[] arguments ) throws Proc
     }
 
     @Override
-    public Object functionCallElevated( QualifiedName name, Object[] arguments ) throws ProcedureException
+    public Object functionCallOverride( QualifiedName name, Object[] arguments ) throws ProcedureException
     {
-        return callFunction( name, arguments, new ElevatedAccessMode( tx.mode(), AccessMode.Static.READ ) );
+        return callFunction( name, arguments, new OverriddenAccessMode( tx.mode(), AccessMode.Static.READ ) );
     }
 
     private Object callFunction(

diff --git a/...impl/api/security/ElevatedAccessMode.java → ...pl/api/security/OverriddenAccessMode.java b/...impl/api/security/ElevatedAccessMode.java → ...pl/api/security/OverriddenAccessMode.java
@@ -21,35 +21,34 @@
 
 import org.neo4j.kernel.api.security.AccessMode;
 
-public class ElevatedAccessMode extends LayeredAccessMode
+public class OverriddenAccessMode extends WrappedAccessMode
 {
-    public ElevatedAccessMode( AccessMode originalMode,
-            AccessMode elevatedTo )
+    public OverriddenAccessMode( AccessMode original, AccessMode overriding )
     {
-        super( originalMode, elevatedTo );
+        super( original, overriding );
     }
 
     @Override
     public boolean allowsReads()
     {
-        return overriddenMode.allowsReads() || originalMode.allowsReads();
+        return wrapping.allowsReads();
     }
 
     @Override
     public boolean allowsWrites()
     {
-        return overriddenMode.allowsWrites() || originalMode.allowsWrites();
+        return wrapping.allowsWrites();
     }
 
     @Override
     public boolean allowsSchemaWrites()
     {
-        return overriddenMode.allowsSchemaWrites() || originalMode.allowsSchemaWrites();
+        return wrapping.allowsSchemaWrites();
     }
 
     @Override
     public String name()
     {
-        return originalMode.name() + " elevated to " + overriddenMode.name();
+        return original.name() + " overridden by " + wrapping.name();
     }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/RestrictedAccessMode.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/RestrictedAccessMode.java
@@ -21,35 +21,34 @@
 
 import org.neo4j.kernel.api.security.AccessMode;
 
-public class RestrictedAccessMode extends LayeredAccessMode
+public class RestrictedAccessMode extends WrappedAccessMode
 {
-    public RestrictedAccessMode( AccessMode originalMode,
-            AccessMode restrictedTo )
+    public RestrictedAccessMode( AccessMode original, AccessMode restricting )
     {
-        super( originalMode, restrictedTo );
+        super( original, restricting );
     }
 
     @Override
     public boolean allowsReads()
     {
-        return overriddenMode.allowsReads() && originalMode.allowsReads();
+        return original.allowsReads() && wrapping.allowsReads();
     }
 
     @Override
     public boolean allowsWrites()
     {
-        return overriddenMode.allowsWrites() && originalMode.allowsWrites();
+        return original.allowsWrites() && wrapping.allowsWrites();
     }
 
     @Override
     public boolean allowsSchemaWrites()
     {
-        return overriddenMode.allowsSchemaWrites() && originalMode.allowsSchemaWrites();
+        return original.allowsSchemaWrites() && wrapping.allowsSchemaWrites();
     }
 
     @Override
     public String name()
     {
-        return originalMode.name() + " restricted to " + overriddenMode.name();
+        return original.name() + " restricted to " + wrapping.name();
     }
 }
diff --git a/.../impl/api/security/LayeredAccessMode.java → .../impl/api/security/WrappedAccessMode.java b/.../impl/api/security/LayeredAccessMode.java → .../impl/api/security/WrappedAccessMode.java
@@ -22,33 +22,33 @@
 import org.neo4j.graphdb.security.AuthorizationViolationException;
 import org.neo4j.kernel.api.security.AccessMode;
 
-abstract class LayeredAccessMode implements AccessMode
+abstract class WrappedAccessMode implements AccessMode
 {
-    protected final AccessMode originalMode;
-    protected final AccessMode overriddenMode;
+    protected final AccessMode original;
+    protected final AccessMode wrapping;
 
-    public LayeredAccessMode( AccessMode originalMode, AccessMode overriddenMode )
+    WrappedAccessMode( AccessMode original, AccessMode wrapping )
     {
-        this.originalMode = originalMode;
-        this.overriddenMode = overriddenMode;
+        this.original = original;
+        this.wrapping = wrapping;
     }
 
     @Override
     public AuthorizationViolationException onViolation( String msg )
     {
-        return overriddenMode.onViolation( msg );
+        return wrapping.onViolation( msg );
     }
 
     @Override
     public String username()
     {
-        return originalMode.username();
+        return original.username();
     }
 
     @Override
     public AccessMode getOriginalAccessMode()
     {
-        return originalMode.getOriginalAccessMode();
+        return original.getOriginalAccessMode();
     }
 
     @Override

diff --git a/.../java/org/neo4j/server/security/enterprise/auth/BuiltInProceduresInteractionTestBase.java b/.../java/org/neo4j/server/security/enterprise/auth/BuiltInProceduresInteractionTestBase.java
@@ -778,6 +778,19 @@ public void shouldFailNestedAllowedWriteProcedureFromAllowedReadProcedure() thro
                 WRITE_OPS_NOT_ALLOWED );
     }
 
+    @Test
+    public void shouldFailNestedAllowedWriteProcedureFromAllowedReadProcedureEvenIfAdmin() throws Throwable
+    {
+        userManager = neo.getLocalUserManager();
+        userManager.newUser( "role1Subject", "abc", false );
+        userManager.newRole( "role1" );
+        userManager.addRoleToUser( "role1", "role1Subject" );
+        userManager.addRoleToUser( PredefinedRoles.ADMIN, "role1Subject" );
+        assertFail( neo.login( "role1Subject", "abc" ),
+                "CALL test.nestedAllowedProcedure('test.allowedWriteProcedure') YIELD value",
+                WRITE_OPS_NOT_ALLOWED );
+    }
+
     @Test
     public void shouldRestrictNestedReadProcedureFromAllowedWriteProcedures() throws Throwable
     {