From eae7dec0ae5421db99da3ca8372b0868589a8a10 Mon Sep 17 00:00:00 2001 From: Mats Rydberg Date: Tue, 4 Oct 2016 21:22:40 +0200 Subject: [PATCH] Refactor procedure call operations Procedures have their own way of dealing with access control, and are not served well by the access check made when acquiring the *Operations interfaces. This commit moves the procedure call methods to a dedicated interface, which can be acquired by any caller. --- .../v3_0/TransactionBoundQueryContext.scala | 4 +- .../v3_1/TransactionBoundQueryContext.scala | 12 +-- .../neo4j/kernel/api/DataWriteOperations.java | 6 -- .../kernel/api/ProcedureCallOperations.java | 95 +++++++++++++++++++ .../org/neo4j/kernel/api/ReadOperations.java | 7 -- .../kernel/api/SchemaWriteOperations.java | 7 -- .../java/org/neo4j/kernel/api/Statement.java | 5 + .../kernel/impl/api/KernelStatement.java | 7 ++ .../kernel/impl/api/OperationsFacade.java | 78 +++++++-------- .../builtinprocs/SchemaProcedureIT.java | 6 +- .../kernel/builtinprocs/StubStatement.java | 7 ++ .../integrationtest/BuiltInProceduresIT.java | 12 +-- .../KernelIntegrationTest.java | 8 ++ .../integrationtest/ProceduresKernelIT.java | 5 +- .../scenarios/ClusterDiscoveryIT.java | 2 +- .../scenarios/ClusterMembershipChangeIT.java | 2 +- .../coreedge/scenarios/ClusterOverviewIT.java | 2 +- .../BuiltInProceduresInteractionTestBase.java | 16 ++++ .../auth/ProcedureInteractionTestBase.java | 22 +++-- 19 files changed, 213 insertions(+), 90 deletions(-) create mode 100644 community/kernel/src/main/java/org/neo4j/kernel/api/ProcedureCallOperations.java diff --git a/community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/spi/v3_0/TransactionBoundQueryContext.scala b/community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/spi/v3_0/TransactionBoundQueryContext.scala index 4777c11102fe6..eeb2c9b80c811 100644 --- a/community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/spi/v3_0/TransactionBoundQueryContext.scala +++ b/community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/spi/v3_0/TransactionBoundQueryContext.scala @@ -617,10 +617,10 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional } override def callReadOnlyProcedure(name: QualifiedProcedureName, args: Seq[Any]) = - callProcedure(name, args, transactionalContext.statement.readOperations().procedureCallRead) + callProcedure(name, args, transactionalContext.statement.procedureCallOperations().procedureCallRead) override def callReadWriteProcedure(name: QualifiedProcedureName, args: Seq[Any]) = - callProcedure(name, args, transactionalContext.statement.dataWriteOperations().procedureCallWrite) + callProcedure(name, args, transactionalContext.statement.procedureCallOperations().procedureCallWrite) override def callDbmsProcedure(name: QualifiedProcedureName, args: Seq[Any]) = callProcedure(name, args, transactionalContext.dbmsOperations.procedureCallDbms(_,_,transactionalContext.accessMode)) 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 1eb155d1eddb4..894cc10d043f7 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 @@ -590,9 +590,9 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional override def callReadOnlyProcedure(name: QualifiedName, args: Seq[Any], allowed: Array[String]) = { val call: KernelProcedureCall = transactionalContext.accessMode match { case a: AuthSubject if a.allowsProcedureWith(allowed) => - transactionalContext.statement.readOperations().procedureCallRead(_, _, AccessMode.Static.OVERRIDE_READ) + transactionalContext.statement.procedureCallOperations.procedureCallRead(_, _, AccessMode.Static.OVERRIDE_READ) case _ => - transactionalContext.statement.readOperations().procedureCallRead(_, _) + transactionalContext.statement.procedureCallOperations.procedureCallRead(_, _) } callProcedure(name, args, call) } @@ -600,9 +600,9 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional override def callReadWriteProcedure(name: QualifiedName, args: Seq[Any], allowed: Array[String]) = { val call: KernelProcedureCall = transactionalContext.accessMode match { case a: AuthSubject if a.allowsProcedureWith(allowed) => - transactionalContext.statement.dataWriteOperations().procedureCallWrite(_, _, AccessMode.Static.OVERRIDE_WRITE) + transactionalContext.statement.procedureCallOperations.procedureCallWrite(_, _, AccessMode.Static.OVERRIDE_WRITE) case _ => - transactionalContext.statement.dataWriteOperations().procedureCallWrite(_, _) + transactionalContext.statement.procedureCallOperations.procedureCallWrite(_, _) } callProcedure(name, args, call) } @@ -610,9 +610,9 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional override def callSchemaWriteProcedure(name: QualifiedName, args: Seq[Any], allowed: Array[String]) = { val call: KernelProcedureCall = transactionalContext.accessMode match { case a: AuthSubject if a.allowsProcedureWith(allowed) => - transactionalContext.statement.schemaWriteOperations().procedureCallSchema(_, _, AccessMode.Static.OVERRIDE_SCHEMA) + transactionalContext.statement.procedureCallOperations.procedureCallSchema(_, _, AccessMode.Static.OVERRIDE_SCHEMA) case _ => - transactionalContext.statement.schemaWriteOperations().procedureCallSchema(_, _) + transactionalContext.statement.procedureCallOperations.procedureCallSchema(_, _) } callProcedure(name, args, call) } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/DataWriteOperations.java b/community/kernel/src/main/java/org/neo4j/kernel/api/DataWriteOperations.java index 4597285f231fb..ae622af4ca9f1 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/DataWriteOperations.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/DataWriteOperations.java @@ -149,10 +149,4 @@ void relationshipRemoveFromLegacyIndex( String indexName, long relationship ) void nodeLegacyIndexDrop( String indexName ) throws LegacyIndexNotFoundKernelException; void relationshipLegacyIndexDrop( String indexName ) throws LegacyIndexNotFoundKernelException; - - /** Invoke a read/write procedure by name */ - RawIterator procedureCallWrite( QualifiedName name, Object[] input ) throws ProcedureException; - - RawIterator procedureCallWrite( QualifiedName name, Object[] input, AccessMode override ) - throws ProcedureException; } 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 new file mode 100644 index 0000000000000..d16710e79696f --- /dev/null +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/ProcedureCallOperations.java @@ -0,0 +1,95 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.kernel.api; + +import org.neo4j.collection.RawIterator; +import org.neo4j.kernel.api.exceptions.ProcedureException; +import org.neo4j.kernel.api.proc.QualifiedName; +import org.neo4j.kernel.api.security.AccessMode; + +/** + * Specifies procedure call operations for the three types of procedure calls that can be made. + */ +public interface ProcedureCallOperations +{ + /** + * Invoke a read-only procedure by name. + * @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 procedureCallRead( QualifiedName name, Object[] arguments ) + throws ProcedureException; + + /** + * Invoke a read-only procedure by name, and override the transaction's access mode with + * the given access mode for the duration of the procedure execution. + * @param name the name of the procedure. + * @param arguments the procedure arguments. + * @param override the access mode to be used for the execution of the procedure. + * @return an iterator containing the procedure results. + * @throws ProcedureException if there was an exception thrown during procedure execution. + */ + RawIterator procedureCallRead( QualifiedName name, Object[] arguments, AccessMode override ) + throws ProcedureException; + + /** + * Invoke a read/write procedure by name. + * @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 procedureCallWrite( QualifiedName name, Object[] arguments ) + throws ProcedureException; + /** + * Invoke a read/write procedure by name, and override the transaction's access mode with + * the given access mode for the duration of the procedure execution. + * @param name the name of the procedure. + * @param arguments the procedure arguments. + * @param override the access mode to be used for the execution of the procedure. + * @return an iterator containing the procedure results. + * @throws ProcedureException if there was an exception thrown during procedure execution. + */ + RawIterator procedureCallWrite( QualifiedName name, Object[] arguments, AccessMode override ) + throws ProcedureException; + + /** + * Invoke a schema write procedure by name. + * @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 procedureCallSchema( QualifiedName name, Object[] arguments ) + throws ProcedureException; + /** + * Invoke a schema write procedure by name, and override the transaction's access mode with + * the given access mode for the duration of the procedure execution. + * @param name the name of the procedure. + * @param arguments the procedure arguments. + * @param override the access mode to be used for the execution of the procedure. + * @return an iterator containing the procedure results. + * @throws ProcedureException if there was an exception thrown during procedure execution. + */ + RawIterator procedureCallSchema( QualifiedName name, Object[] arguments, AccessMode override ) + throws ProcedureException; +} diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/ReadOperations.java b/community/kernel/src/main/java/org/neo4j/kernel/api/ReadOperations.java index 1b12ba29e8926..c7d3853be8327 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/ReadOperations.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/ReadOperations.java @@ -567,13 +567,6 @@ DoubleLongRegister indexSample( IndexDescriptor index, DoubleLongRegister target /** Fetch all registered procedures */ Set proceduresGetAll(); - /** Invoke a read-only procedure by name */ - RawIterator procedureCallRead( QualifiedName name, Object[] input ) - throws ProcedureException; - - RawIterator procedureCallRead( QualifiedName name, Object[] input, AccessMode override ) - throws ProcedureException; - /** Invoke a read-only procedure by name */ Object functionCall( QualifiedName name, Object[] input ) throws ProcedureException; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/SchemaWriteOperations.java b/community/kernel/src/main/java/org/neo4j/kernel/api/SchemaWriteOperations.java index a6f962ec5ed50..7f43747becb73 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/SchemaWriteOperations.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/SchemaWriteOperations.java @@ -65,11 +65,4 @@ RelationshipPropertyExistenceConstraint relationshipPropertyExistenceConstraintC * That external job should become an internal job, at which point this operation should go away. */ void uniqueIndexDrop( IndexDescriptor descriptor ) throws DropIndexFailureException; - - /** Invoke a schema procedure by name */ - RawIterator procedureCallSchema( QualifiedName name, Object[] input ) - throws ProcedureException; - - RawIterator procedureCallSchema( QualifiedName name, Object[] input, AccessMode override ) - throws ProcedureException; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/Statement.java b/community/kernel/src/main/java/org/neo4j/kernel/api/Statement.java index 335adbcdcec2f..6c97fb36bd794 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/Statement.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/Statement.java @@ -63,4 +63,9 @@ public interface Statement extends Resource * @return interface exposing operations for associating metadata with this statement */ QueryRegistryOperations queryRegistration(); + + /** + * @return interface exposing all procedure call operations. + */ + ProcedureCallOperations procedureCallOperations(); } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelStatement.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelStatement.java index e01bbe57c9772..c3031f967c200 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelStatement.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelStatement.java @@ -23,6 +23,7 @@ import org.neo4j.graphdb.NotInTransactionException; import org.neo4j.graphdb.TransactionTerminatedException; +import org.neo4j.kernel.api.ProcedureCallOperations; import org.neo4j.kernel.api.DataWriteOperations; import org.neo4j.kernel.api.ExecutingQuery; import org.neo4j.kernel.api.QueryRegistryOperations; @@ -97,6 +98,12 @@ public ReadOperations readOperations() return facade; } + @Override + public ProcedureCallOperations procedureCallOperations() + { + return facade; + } + @Override public TokenWriteOperations tokenWriteOperations() { 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 a33d8ff551be6..ac430417990ce 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 @@ -33,6 +33,7 @@ import org.neo4j.collection.primitive.PrimitiveLongIterator; import org.neo4j.cursor.Cursor; import org.neo4j.graphdb.Direction; +import org.neo4j.kernel.api.ProcedureCallOperations; import org.neo4j.kernel.api.DataWriteOperations; import org.neo4j.kernel.api.ExecutingQuery; import org.neo4j.kernel.api.KernelTransaction; @@ -101,7 +102,8 @@ import org.neo4j.storageengine.api.schema.PopulationProgress; public class OperationsFacade - implements ReadOperations, DataWriteOperations, SchemaWriteOperations, QueryRegistryOperations + implements ReadOperations, DataWriteOperations, SchemaWriteOperations, QueryRegistryOperations, + ProcedureCallOperations { private final KernelTransaction tx; private final KernelStatement statement; @@ -571,18 +573,6 @@ public Object functionCall( QualifiedName name, Object[] input ) throws Procedur return callFunction( name, input ); } - @Override - public RawIterator procedureCallRead( QualifiedName name, Object[] input ) throws ProcedureException - { - return callProcedure( name, input, AccessMode.Static.READ ); - } - - @Override - public RawIterator procedureCallRead( QualifiedName name, Object[] input, AccessMode override ) throws ProcedureException - { - return callProcedure( name, input, override ); - } - @Override public Set proceduresGetAll() { @@ -1095,19 +1085,6 @@ public Property graphRemoveProperty( int propertyKeyId ) return dataWrite().graphRemoveProperty( statement, propertyKeyId ); } - @Override - public RawIterator procedureCallWrite( QualifiedName name, Object[] input ) throws ProcedureException - { - // FIXME: should this be AccessMode.Static.WRITE instead? - return callProcedure( name, input, AccessMode.Static.FULL ); - } - - @Override - public RawIterator procedureCallWrite( QualifiedName name, Object[] input, AccessMode override ) throws ProcedureException - { - return callProcedure( name, input, override ); - } - // // @@ -1172,18 +1149,6 @@ public void uniqueIndexDrop( IndexDescriptor descriptor ) throws DropIndexFailur schemaWrite().uniqueIndexDrop( statement, descriptor ); } - @Override - public RawIterator procedureCallSchema( QualifiedName name, Object[] input ) throws ProcedureException - { - return callProcedure( name, input, AccessMode.Static.FULL ); - } - - @Override - public RawIterator procedureCallSchema( QualifiedName name, Object[] input, AccessMode override ) throws ProcedureException - { - return callProcedure( name, input, override ); - } - // // @@ -1528,6 +1493,43 @@ public void unregisterExecutingQuery( ExecutingQuery executingQuery ) // + @Override + public RawIterator procedureCallRead( QualifiedName name, Object[] input ) throws ProcedureException + { + return callProcedure( name, input, AccessMode.Static.READ ); + } + + @Override + public RawIterator procedureCallRead( QualifiedName name, Object[] input, AccessMode override ) throws ProcedureException + { + return callProcedure( name, input, override ); + } + + @Override + public RawIterator procedureCallWrite( QualifiedName name, Object[] input ) throws ProcedureException + { + // FIXME: should this be AccessMode.Static.WRITE instead? + return callProcedure( name, input, AccessMode.Static.FULL ); + } + + @Override + public RawIterator procedureCallWrite( QualifiedName name, Object[] input, AccessMode override ) throws ProcedureException + { + return callProcedure( name, input, override ); + } + + @Override + public RawIterator procedureCallSchema( QualifiedName name, Object[] input ) throws ProcedureException + { + return callProcedure( name, input, AccessMode.Static.FULL ); + } + + @Override + public RawIterator procedureCallSchema( QualifiedName name, Object[] input, AccessMode override ) throws ProcedureException + { + return callProcedure( name, input, override ); + } + private RawIterator callProcedure( QualifiedName name, Object[] input, AccessMode mode ) throws ProcedureException diff --git a/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/SchemaProcedureIT.java b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/SchemaProcedureIT.java index 93801cb2a3fcb..a3ddf24cff694 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/SchemaProcedureIT.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/SchemaProcedureIT.java @@ -58,7 +58,7 @@ public void testEmptyGraph() throws Throwable // When RawIterator stream = - readOperationsInNewTransaction().procedureCallRead( procedureName( "db", "schema" ), new Object[0] ); + procedureCallOpsInNewTx().procedureCallRead( procedureName( "db", "schema" ), new Object[0] ); // Then assertThat( asList( stream ), contains( equalTo( new Object[]{new ArrayList<>(), new ArrayList<>()} ) ) ); @@ -84,7 +84,7 @@ public void testLabelIndex() throws Throwable // When RawIterator stream = - readOperationsInNewTransaction().procedureCallRead( procedureName( "db", "schema" ), new Object[0] ); + procedureCallOpsInNewTx().procedureCallRead( procedureName( "db", "schema" ), new Object[0] ); // Then while ( stream.hasNext() ) @@ -117,7 +117,7 @@ public void testRelationShip() throws Throwable // When RawIterator stream = - readOperationsInNewTransaction().procedureCallRead( procedureName( "db", "schema" ), new Object[0] ); + procedureCallOpsInNewTx().procedureCallRead( procedureName( "db", "schema" ), new Object[0] ); // Then while ( stream.hasNext() ) diff --git a/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/StubStatement.java b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/StubStatement.java index 0147f2e940e49..099aa94f3a265 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/StubStatement.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/StubStatement.java @@ -20,6 +20,7 @@ package org.neo4j.kernel.builtinprocs; import org.neo4j.kernel.api.DataWriteOperations; +import org.neo4j.kernel.api.ProcedureCallOperations; import org.neo4j.kernel.api.QueryRegistryOperations; import org.neo4j.kernel.api.ReadOperations; import org.neo4j.kernel.api.SchemaWriteOperations; @@ -71,4 +72,10 @@ public QueryRegistryOperations queryRegistration() { throw new UnsupportedOperationException( "not implemented" ); } + + @Override + public ProcedureCallOperations procedureCallOperations() + { + throw new UnsupportedOperationException( "not implemented" ); + } } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/BuiltInProceduresIT.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/BuiltInProceduresIT.java index 4313d668b0b34..7d58e7348bf79 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/BuiltInProceduresIT.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/BuiltInProceduresIT.java @@ -57,7 +57,7 @@ public void listAllLabels() throws Throwable // When RawIterator stream = - readOperationsInNewTransaction().procedureCallRead( procedureName( "db", "labels" ), new Object[0] ); + procedureCallOpsInNewTx().procedureCallRead( procedureName( "db", "labels" ), new Object[0] ); // Then assertThat( asList( stream ), contains( equalTo( new Object[]{"MyLabel"} ) ) ); @@ -72,7 +72,7 @@ public void listPropertyKeys() throws Throwable commit(); // When - RawIterator stream = readOperationsInNewTransaction() + RawIterator stream = procedureCallOpsInNewTx() .procedureCallRead( procedureName( "db", "propertyKeys" ), new Object[0] ); // Then @@ -89,7 +89,7 @@ public void listRelationshipTypes() throws Throwable commit(); // When - RawIterator stream = readOperationsInNewTransaction() + RawIterator stream = procedureCallOpsInNewTx() .procedureCallRead( procedureName( "db", "relationshipTypes" ), new Object[0] ); // Then @@ -100,7 +100,7 @@ public void listRelationshipTypes() throws Throwable public void listProcedures() throws Throwable { // When - RawIterator stream = readOperationsInNewTransaction() + RawIterator stream = procedureCallOpsInNewTx() .procedureCallRead( procedureName( "dbms", "procedures" ), new Object[0] ); // Then @@ -154,7 +154,7 @@ public void listAllComponents() throws Throwable // Given a running database // When - RawIterator stream = readOperationsInNewTransaction() + RawIterator stream = procedureCallOpsInNewTx() .procedureCallRead( procedureName( "dbms", "components" ), new Object[0] ); // Then @@ -183,7 +183,7 @@ public void listAllIndexes() throws Throwable // When RawIterator stream = - readOperationsInNewTransaction().procedureCallRead( procedureName( "db", "indexes" ), new Object[0] ); + procedureCallOpsInNewTx().procedureCallRead( procedureName( "db", "indexes" ), new Object[0] ); // Then assertThat( stream.next(), equalTo( new Object[]{"INDEX ON :Age(foo)", "ONLINE", diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/KernelIntegrationTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/KernelIntegrationTest.java index fcf80d3b553f1..5703f2a8eb3c6 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/KernelIntegrationTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/KernelIntegrationTest.java @@ -27,6 +27,7 @@ import org.neo4j.kernel.api.DataWriteOperations; import org.neo4j.kernel.api.KernelAPI; import org.neo4j.kernel.api.KernelTransaction; +import org.neo4j.kernel.api.ProcedureCallOperations; import org.neo4j.kernel.api.ReadOperations; import org.neo4j.kernel.api.SchemaWriteOperations; import org.neo4j.kernel.api.Statement; @@ -75,6 +76,13 @@ protected SchemaWriteOperations schemaWriteOperationsInNewTransaction() throws K return statement.schemaWriteOperations(); } + protected ProcedureCallOperations procedureCallOpsInNewTx() throws TransactionFailureException + { + transaction = kernel.newTransaction( KernelTransaction.Type.implicit, AccessMode.Static.READ ); + statement = transaction.acquireStatement(); + return statement.procedureCallOperations(); + } + protected ReadOperations readOperationsInNewTransaction() throws TransactionFailureException { transaction = kernel.newTransaction( KernelTransaction.Type.implicit, AccessMode.Static.READ ); diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/ProceduresKernelIT.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/ProceduresKernelIT.java index 7d4f4946df72e..1f81ba91ad8b9 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/ProceduresKernelIT.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/ProceduresKernelIT.java @@ -30,7 +30,6 @@ import org.neo4j.kernel.api.exceptions.ProcedureException; import org.neo4j.kernel.api.proc.CallableProcedure; import org.neo4j.kernel.api.proc.Context; -import org.neo4j.kernel.api.proc.Neo4jTypes; import org.neo4j.kernel.api.proc.QualifiedName; import org.neo4j.kernel.api.proc.ProcedureSignature; @@ -118,7 +117,7 @@ public void shouldCallReadOnlyProcedure() throws Throwable kernel.registerProcedure( procedure ); // When - RawIterator found = readOperationsInNewTransaction() + RawIterator found = procedureCallOpsInNewTx() .procedureCallRead( new QualifiedName( new String[]{"example"}, "exampleProc" ), new Object[]{ 1337 } ); // Then @@ -139,7 +138,7 @@ public RawIterator apply( Context ctx, Object[] in } ); // When - RawIterator stream = readOperationsInNewTransaction().procedureCallRead( signature.name(), new Object[]{""} ); + RawIterator stream = procedureCallOpsInNewTx().procedureCallRead( signature.name(), new Object[]{""} ); // Then assertNotNull( asList( stream ).get( 0 )[0] ); diff --git a/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterDiscoveryIT.java b/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterDiscoveryIT.java index 722420fa9d04a..b4a2ecdb252ea 100644 --- a/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterDiscoveryIT.java +++ b/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterDiscoveryIT.java @@ -107,7 +107,7 @@ private List getMembers( GraphDatabaseFacade db ) throws TransactionFa try ( Statement statement = transaction.acquireStatement() ) { // when - return asList( statement.readOperations().procedureCallRead( + return asList( statement.procedureCallOperations().procedureCallRead( procedureName( "dbms", "cluster", "routing", GetServersProcedure.NAME ), new Object[0] ) ); } diff --git a/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterMembershipChangeIT.java b/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterMembershipChangeIT.java index cb3993c2a7bdd..f895fe8250749 100644 --- a/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterMembershipChangeIT.java +++ b/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterMembershipChangeIT.java @@ -91,7 +91,7 @@ private List discoverClusterMembers( GraphDatabaseFacade db ) Statement statement = transaction.acquireStatement(); // when - return asList( statement.readOperations().procedureCallRead( + return asList( statement.procedureCallOperations().procedureCallRead( procedureName( "dbms", "cluster", GetServersProcedure.NAME ), new Object[0] ) ); } } diff --git a/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterOverviewIT.java b/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterOverviewIT.java index d45907dd64889..11be393892bab 100644 --- a/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterOverviewIT.java +++ b/enterprise/core-edge/src/test/java/org/neo4j/coreedge/scenarios/ClusterOverviewIT.java @@ -339,7 +339,7 @@ private List clusterOverview( GraphDatabaseFacade db ) try ( Statement statement = transaction.acquireStatement() ) { - RawIterator itr = statement.readOperations().procedureCallRead( + RawIterator itr = statement.procedureCallOperations().procedureCallRead( procedureName( "dbms", "cluster", ClusterOverviewProcedure.PROCEDURE_NAME ), null ); while ( itr.hasNext() ) 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 660fec074b122..d1bde8cae91b7 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 @@ -67,6 +67,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.isA; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.neo4j.graphdb.security.AuthorizationViolationException.PERMISSION_DENIED; import static org.neo4j.helpers.collection.Iterables.single; @@ -689,6 +690,21 @@ public void shouldHandleWriteAfterAllowedReadProcedureForWriteUser() throws Thro assertEmpty( neo.login( "role1Subject", "abc" ), "CALL test.allowedProcedure1() YIELD value CREATE (:NEWNODE {name:value})" ); } + @Test + public void shouldNotAllowNonWriterToWriteAfterCallingAllowedWriteProc() throws Exception + { + userManager = neo.getLocalUserManager(); + userManager.newUser( "nopermission", "abc", false ); + userManager.newRole( "role1" ); + userManager.addRoleToUser( "role1", "nopermission" ); + // should be able to invoke allowed procedure + assertSuccess( neo.login( "nopermission", "abc" ), "CALL test.allowedProcedure2()", + itr -> assertEquals( itr.stream().collect( toList() ).size(), 2 ) ); + // should not be able to do writes + assertFail( neo.login( "nopermission", "abc" ), + "CALL test.allowedProcedure2() YIELD value CREATE (:NEWNODE {name:value})", WRITE_OPS_NOT_ALLOWED ); + } + /* This surface is hidden in 3.1, to possibly be completely removed or reworked later ================================================================================== diff --git a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/ProcedureInteractionTestBase.java b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/ProcedureInteractionTestBase.java index 364475af32ddd..467e14ebcd672 100644 --- a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/ProcedureInteractionTestBase.java +++ b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/ProcedureInteractionTestBase.java @@ -41,6 +41,7 @@ import org.neo4j.bolt.v1.transport.socket.client.TransportConnection; import org.neo4j.graphdb.GraphDatabaseService; import org.neo4j.graphdb.ResourceIterator; +import org.neo4j.graphdb.Result; import org.neo4j.graphdb.factory.GraphDatabaseSettings; import org.neo4j.graphdb.security.AuthorizationViolationException; import org.neo4j.helpers.HostnamePort; @@ -75,13 +76,16 @@ import static org.neo4j.kernel.impl.api.security.OverriddenAccessMode.getUsernameFromAccessMode; import static org.neo4j.procedure.Mode.READ; import static org.neo4j.procedure.Mode.WRITE; -import static org.neo4j.server.security.enterprise.auth.plugin.api.PredefinedRoles.*; +import static org.neo4j.server.security.enterprise.auth.plugin.api.PredefinedRoles.ADMIN; +import static org.neo4j.server.security.enterprise.auth.plugin.api.PredefinedRoles.ARCHITECT; +import static org.neo4j.server.security.enterprise.auth.plugin.api.PredefinedRoles.PUBLISHER; +import static org.neo4j.server.security.enterprise.auth.plugin.api.PredefinedRoles.READER; public abstract class ProcedureInteractionTestBase { protected boolean PWD_CHANGE_CHECK_FIRST = false; protected String CHANGE_PWD_ERR_MSG = AuthorizationViolationException.PERMISSION_DENIED; - private String BOLT_PWD_ERR_MSG = + private static final String BOLT_PWD_ERR_MSG = "The credentials you provided were valid, but must be changed before you can use this instance."; String READ_OPS_NOT_ALLOWED = "Read operations are not allowed"; String WRITE_OPS_NOT_ALLOWED = "Write operations are not allowed"; @@ -300,7 +304,7 @@ void testSuccessfulTestProcs( S subject ) assertSuccess( subject, "CALL test.allowedProcedure1()", r -> assertKeyIs( r, "value", "foo" ) ); assertSuccess( subject, "CALL test.allowedProcedure2()", - r -> assertKeyIs( r, "value", "a" ) ); + r -> assertKeyIs( r, "value", "a", "a" ) ); assertSuccess( subject, "CALL test.allowedProcedure3()", r -> assertKeyIs( r, "value", "OK" ) ); } @@ -420,7 +424,7 @@ private void assertKeyIsArray( ResourceIterator> r, String ke @SuppressWarnings( "unchecked" ) public static void assertKeyIsMap( ResourceIterator> r, String keyKey, String valueKey, Map expected ) { - List> result = r.stream().collect( Collectors.toList() ); + List> result = r.stream().collect( toList() ); assertEquals( "Results for should have size " + expected.size() + " but was " + result.size(), expected.size(), result.size() ); @@ -540,16 +544,16 @@ public Stream numNodes() @Procedure( name = "test.allowedProcedure1", allowed = {"role1"}, mode = Mode.READ ) public Stream allowedProcedure1() { - db.execute( "MATCH (:Foo) RETURN 'foo' AS foo" ); - return Stream.of( new AuthProceduresBase.StringResult( "foo" ) ); + Result result = db.execute( "MATCH (:Foo) WITH count(*) AS c RETURN 'foo' AS foo" ); + return result.stream().map( r -> new AuthProceduresBase.StringResult( r.get( "foo" ).toString() ) ); } @Procedure( name = "test.allowedProcedure2", allowed = {"otherRole", "role1"}, mode = Mode.WRITE ) public Stream allowedProcedure2() { - db.execute( "CREATE (:VeryUniqueLabel {prop: 'a'})" ); - return db.execute( "MATCH (n:VeryUniqueLabel) RETURN n.prop AS a LIMIT 1" ).stream() - .map( r -> new AuthProceduresBase.StringResult( (String) r.get( "a" ) ) ); + db.execute( "UNWIND [1, 2] AS i CREATE (:VeryUniqueLabel {prop: 'a'})" ); + Result result = db.execute( "MATCH (n:VeryUniqueLabel) RETURN n.prop AS a LIMIT 2" ); + return result.stream().map( r -> new AuthProceduresBase.StringResult( r.get( "a" ).toString() ) ); } @Procedure( name = "test.allowedProcedure3", allowed = {"role1"}, mode = Mode.SCHEMA )