From 2a08c9b89a288f3f028633346b940c6aab540433 Mon Sep 17 00:00:00 2001 From: Mats Rydberg Date: Wed, 12 Oct 2016 11:07:51 +0200 Subject: [PATCH] Fix bug with elevating access mode - Modify javadocs - Replace concept of elevated access mode with overridden access mode --- .../v3_1/TransactionBoundQueryContext.scala | 8 +++---- .../kernel/api/ProcedureCallOperations.java | 24 +++++++++---------- .../kernel/impl/api/OperationsFacade.java | 18 +++++++------- ...essMode.java => OverriddenAccessMode.java} | 15 ++++++------ .../api/security/RestrictedAccessMode.java | 15 ++++++------ ...AccessMode.java => WrappedAccessMode.java} | 18 +++++++------- .../BuiltInProceduresInteractionTestBase.java | 13 ++++++++++ 7 files changed, 61 insertions(+), 50 deletions(-) rename community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/{ElevatedAccessMode.java => OverriddenAccessMode.java} (69%) rename community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/{LayeredAccessMode.java => WrappedAccessMode.java} (75%) diff --git a/community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/spi/v3_1/TransactionBoundQueryContext.scala b/community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/spi/v3_1/TransactionBoundQueryContext.scala index 6d5a63596c3db..e14606b42cab6 100644 --- a/community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/spi/v3_1/TransactionBoundQueryContext.scala +++ b/community/cypher/cypher/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 index 62fe27c787255..37ccb84b7854f 100644 --- 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 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 procedureCallReadElevated( QualifiedName name, Object[] arguments ) + RawIterator procedureCallReadOverride( QualifiedName name, Object[] arguments ) throws ProcedureException; /** @@ -59,14 +59,14 @@ RawIterator procedureCallReadElevated( QualifiedNa RawIterator 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 procedureCallWriteElevated( QualifiedName name, Object[] arguments ) + RawIterator procedureCallWriteOverride( QualifiedName name, Object[] arguments ) throws ProcedureException; /** @@ -79,14 +79,14 @@ RawIterator procedureCallWriteElevated( QualifiedN RawIterator 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 procedureCallSchemaElevated( QualifiedName name, Object[] arguments ) + RawIterator procedureCallSchemaOverride( QualifiedName name, Object[] arguments ) throws ProcedureException; /** Invoke a read-only function by name @@ -96,11 +96,11 @@ RawIterator 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 index 4c628db54ad98..fdb33ab052273 100644 --- 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 procedureCallRead( QualifiedNam } @Override - public RawIterator procedureCallReadElevated( QualifiedName name, Object[] input ) throws ProcedureException + public RawIterator 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 procedureCallWrite( QualifiedNa } @Override - public RawIterator procedureCallWriteElevated( QualifiedName name, Object[] input ) throws ProcedureException + public RawIterator 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 procedureCallSchema( QualifiedN } @Override - public RawIterator procedureCallSchemaElevated( QualifiedName name, Object[] input ) throws ProcedureException + public RawIterator 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 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/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/ElevatedAccessMode.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/OverriddenAccessMode.java similarity index 69% rename from community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/ElevatedAccessMode.java rename to community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/OverriddenAccessMode.java index ef90bd4b9e4bd..d271f91126228 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/ElevatedAccessMode.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/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 index d80b4b7da5716..4853e1923559d 100644 --- 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/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/LayeredAccessMode.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/WrappedAccessMode.java similarity index 75% rename from community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/LayeredAccessMode.java rename to community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/WrappedAccessMode.java index 7d3bd741c13e2..099ad1510f53b 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/LayeredAccessMode.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/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/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/BuiltInProceduresInteractionTestBase.java b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/BuiltInProceduresInteractionTestBase.java index aa895b14854c6..59016eef08377 100644 --- a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/BuiltInProceduresInteractionTestBase.java +++ b/enterprise/security/src/test/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 {