From 8b246b723639026576066b5feefd6ae1db9a130d Mon Sep 17 00:00:00 2001 From: Henrik Nyman Date: Sat, 8 Oct 2016 16:52:35 +0200 Subject: [PATCH] Handle recursive overridden AccessMode - Move getOriginalAccessMode() to AccessMode base class - Move username() to AccessMode base class to get rid of static downcast method. When we have a proper security context we can remove it again. --- .../v3_1/TransactionBoundQueryContext.scala | 16 ++--------- .../neo4j/kernel/api/security/AccessMode.java | 10 +++++++ .../kernel/api/security/AuthSubject.java | 1 + .../kernel/impl/api/KernelStatement.java | 4 +-- .../dbms/NonTransactionalDbmsOperations.java | 5 +--- .../impl/api/security/AccessModeSnapshot.java | 28 ++++++++++++++----- .../api/security/OverriddenAccessMode.java | 28 +++---------------- .../builtinprocs/BuiltInProcedures.java | 5 ++-- .../enterprise/auth/AuthProceduresBase.java | 3 +- .../auth/ProcedureInteractionTestBase.java | 3 +- 10 files changed, 45 insertions(+), 58 deletions(-) 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 e48ffbc7daffb..0d64e7b44ce06 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 @@ -53,7 +53,6 @@ import org.neo4j.kernel.api.exceptions.schema.{AlreadyConstrainedException, Alre import org.neo4j.kernel.api.index.{IndexDescriptor, InternalIndexState} import org.neo4j.kernel.api.proc.{QualifiedName => KernelQualifiedName} import org.neo4j.kernel.api.security.{AccessMode, AuthSubject} -import org.neo4j.kernel.impl.api.security.{AccessModeSnapshot, OverriddenAccessMode} import org.neo4j.kernel.impl.core.NodeManager import org.neo4j.kernel.impl.locking.ResourceTypes @@ -588,17 +587,8 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional type KernelProcedureCall = (KernelQualifiedName, Array[AnyRef]) => RawIterator[Array[AnyRef], ProcedureException] - private def originalAccessMode: Object = { - // TODO: Write a test for recursive procedure calls with allowed annotation - transactionalContext.accessMode match { - case a: AccessModeSnapshot => a.getOriginalAccessMode - case a: OverriddenAccessMode => a.getOriginalAccessMode - case a => a - } - } - override def callReadOnlyProcedure(name: QualifiedName, args: Seq[Any], allowed: Array[String]) = { - val call: KernelProcedureCall = originalAccessMode match { + val call: KernelProcedureCall = transactionalContext.accessMode.getOriginalAccessMode match { case a: AuthSubject if a.allowsProcedureWith(allowed) => transactionalContext.statement.procedureCallOperations.procedureCallRead(_, _, AccessMode.Static.OVERRIDE_READ) case _ => @@ -608,7 +598,7 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional } override def callReadWriteProcedure(name: QualifiedName, args: Seq[Any], allowed: Array[String]) = { - val call: KernelProcedureCall = originalAccessMode match { + val call: KernelProcedureCall = transactionalContext.accessMode.getOriginalAccessMode match { case a: AuthSubject if a.allowsProcedureWith(allowed) => transactionalContext.statement.procedureCallOperations.procedureCallWrite(_, _, AccessMode.Static.OVERRIDE_WRITE) case _ => @@ -618,7 +608,7 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional } override def callSchemaWriteProcedure(name: QualifiedName, args: Seq[Any], allowed: Array[String]) = { - val call: KernelProcedureCall = originalAccessMode match { + val call: KernelProcedureCall = transactionalContext.accessMode.getOriginalAccessMode match { case a: AuthSubject if a.allowsProcedureWith(allowed) => transactionalContext.statement.procedureCallOperations.procedureCallSchema(_, _, AccessMode.Static.OVERRIDE_SCHEMA) case _ => diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/security/AccessMode.java b/community/kernel/src/main/java/org/neo4j/kernel/api/security/AccessMode.java index 33d0518acaab2..ac8ee09dc0509 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/security/AccessMode.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/security/AccessMode.java @@ -366,4 +366,14 @@ public AuthorizationViolationException onViolation( String msg ) boolean overrideOriginalMode(); AuthorizationViolationException onViolation( String msg ); String name(); + + default String username() + { + return ""; // Should never clash with a valid username + } + + default AccessMode getOriginalAccessMode() + { + return this; + } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/security/AuthSubject.java b/community/kernel/src/main/java/org/neo4j/kernel/api/security/AuthSubject.java index 6c3dd6c442bd5..f631601fe5822 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/security/AuthSubject.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/security/AuthSubject.java @@ -57,6 +57,7 @@ public interface AuthSubject extends AccessMode /** * @return A string representing the primary principal of this subject */ + @Override String username(); /** 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 f03fcd449ce2f..337d19bffd063 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 @@ -42,8 +42,6 @@ import org.neo4j.kernel.impl.proc.Procedures; import org.neo4j.storageengine.api.StorageStatement; -import static org.neo4j.kernel.impl.api.security.OverriddenAccessMode.getUsernameFromAccessMode; - /** * A resource efficient implementation of {@link Statement}. Designed to be reused within a * {@link KernelTransactionImplementation} instance, even across transactions since this instances itself @@ -216,7 +214,7 @@ final void forceClose() final Optional username() { - String username = getUsernameFromAccessMode( transaction.mode() ); + String username = transaction.mode().username(); return Optional.of( username ); } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/dbms/NonTransactionalDbmsOperations.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/dbms/NonTransactionalDbmsOperations.java index edebbdfaf8074..0d798b95bf7cd 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/dbms/NonTransactionalDbmsOperations.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/dbms/NonTransactionalDbmsOperations.java @@ -27,7 +27,6 @@ import org.neo4j.kernel.api.proc.QualifiedName; import org.neo4j.kernel.api.security.AccessMode; import org.neo4j.kernel.api.security.AuthSubject; -import org.neo4j.kernel.impl.api.security.AccessModeSnapshot; import org.neo4j.kernel.impl.proc.Procedures; public class NonTransactionalDbmsOperations implements DbmsOperations @@ -48,10 +47,8 @@ public RawIterator procedureCallDbms( ) throws ProcedureException { BasicContext ctx = new BasicContext(); - AccessMode originalMode = (mode instanceof AccessModeSnapshot) ? - ((AccessModeSnapshot) mode).getOriginalAccessMode() : - mode; + AccessMode originalMode = mode.getOriginalAccessMode(); if ( originalMode instanceof AuthSubject ) { ctx.put( Context.AUTH_SUBJECT, (AuthSubject) originalMode ); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/AccessModeSnapshot.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/AccessModeSnapshot.java index 6c14cd5117ee8..137ad10480fe8 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/AccessModeSnapshot.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/AccessModeSnapshot.java @@ -29,10 +29,12 @@ public class AccessModeSnapshot implements AccessMode private final boolean allowsSchemaWrites; private final boolean overrideOriginalMode; - private final AccessMode accessMode; + private final AccessMode originalMode; public static AccessMode createAccessModeSnapshot( AccessMode accessMode ) { + // TODO: Use flyweight pattern instead of always creating new objects, when we have a proper + // security context and do not need to obtain the original mode through this object return new AccessModeSnapshot( accessMode ); } @@ -43,10 +45,13 @@ private AccessModeSnapshot( AccessMode accessMode ) allowsSchemaWrites = accessMode.allowsSchemaWrites(); overrideOriginalMode = accessMode.overrideOriginalMode(); - // We use this for onViolation() and name() - this.accessMode = accessMode; + // We use this for delegation of all the remaining methods + this.originalMode = accessMode; } + //--------------------------------------- + // Snapshot permissions + @Override public boolean allowsReads() { @@ -71,21 +76,30 @@ public boolean overrideOriginalMode() return overrideOriginalMode; } + //--------------------------------------- + // Delegate remaining methods to original + @Override public AuthorizationViolationException onViolation( String msg ) { - return accessMode.onViolation( msg ); + return originalMode.onViolation( msg ); } @Override public String name() { - return accessMode.name(); + return originalMode.name(); } - // TODO: Move this to AccessMode interface with default implementation to support recursive case + @Override + public String username() + { + return originalMode.username(); + } + + @Override public AccessMode getOriginalAccessMode() { - return accessMode; + return originalMode.getOriginalAccessMode(); } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/OverriddenAccessMode.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/OverriddenAccessMode.java index 26fdb72b9478c..6633e04b3db7e 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/OverriddenAccessMode.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/security/OverriddenAccessMode.java @@ -21,7 +21,6 @@ import org.neo4j.graphdb.security.AuthorizationViolationException; import org.neo4j.kernel.api.security.AccessMode; -import org.neo4j.kernel.api.security.AuthSubject; public class OverriddenAccessMode implements AccessMode { @@ -80,35 +79,16 @@ public String name() } } + @Override public String username() { - return getUsernameFromAccessMode( originalMode ); + return originalMode.username(); } - // TODO: Move this to AccessMode interface with default implementation to support recursive case - // OR move allowsProcedureWith() to AccessMode and override that here with recursive implementation + @Override public AccessMode getOriginalAccessMode() { - return originalMode; + return originalMode.getOriginalAccessMode(); } - public static String getUsernameFromAccessMode( AccessMode accessMode ) - { - if ( accessMode instanceof AuthSubject ) - { - return ((AuthSubject) accessMode).username(); - } - else if ( accessMode instanceof OverriddenAccessMode ) - { - return ((OverriddenAccessMode) accessMode).username(); - } - else if ( accessMode instanceof AccessModeSnapshot ) - { - return getUsernameFromAccessMode( ((AccessModeSnapshot) accessMode).getOriginalAccessMode() ); - } - else - { - return ""; // Should never clash with a valid username - } - } } diff --git a/enterprise/kernel/src/main/java/org/neo4j/kernel/enterprise/builtinprocs/BuiltInProcedures.java b/enterprise/kernel/src/main/java/org/neo4j/kernel/enterprise/builtinprocs/BuiltInProcedures.java index 0e5474a184f97..d393ecc1d231f 100644 --- a/enterprise/kernel/src/main/java/org/neo4j/kernel/enterprise/builtinprocs/BuiltInProcedures.java +++ b/enterprise/kernel/src/main/java/org/neo4j/kernel/enterprise/builtinprocs/BuiltInProcedures.java @@ -66,7 +66,6 @@ import static org.neo4j.graphdb.security.AuthorizationViolationException.PERMISSION_DENIED; import static org.neo4j.kernel.enterprise.builtinprocs.QueryId.fromExternalString; import static org.neo4j.kernel.enterprise.builtinprocs.QueryId.ofInternalId; -import static org.neo4j.kernel.impl.api.security.OverriddenAccessMode.getUsernameFromAccessMode; import static org.neo4j.procedure.Mode.DBMS; @SuppressWarnings( "unused" ) @@ -123,7 +122,7 @@ public Stream listTransactions() getActiveTransactions( graph.getDependencyResolver() ) .stream() .filter( tx -> !tx.terminationReason().isPresent() ) - .map( tx -> getUsernameFromAccessMode( tx.mode() ) ) + .map( tx -> tx.mode().username() ) ); } @@ -280,7 +279,7 @@ public static Stream terminateTransactionsForValid { long terminatedCount = getActiveTransactions( dependencyResolver ) .stream() - .filter( tx -> getUsernameFromAccessMode( tx.mode() ).equals( username ) && + .filter( tx -> tx.mode().username().equals( username ) && !tx.isUnderlyingTransaction( currentTx ) ) .map( tx -> tx.markForTermination( Status.Transaction.Terminated ) ) .filter( marked -> marked ) diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProceduresBase.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProceduresBase.java index df1c86f940800..44ee2ba1c7c4b 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProceduresBase.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProceduresBase.java @@ -39,7 +39,6 @@ import static java.util.Collections.emptyList; import static org.neo4j.graphdb.security.AuthorizationViolationException.PERMISSION_DENIED; -import static org.neo4j.kernel.impl.api.security.OverriddenAccessMode.getUsernameFromAccessMode; @SuppressWarnings( "WeakerAccess" ) public class AuthProceduresBase @@ -79,7 +78,7 @@ protected void terminateTransactionsForValidUser( String username ) getActiveTransactions() .stream() .filter( tx -> - getUsernameFromAccessMode( tx.mode() ).equals( username ) && + tx.mode().username().equals( username ) && !tx.isUnderlyingTransaction( currentTx ) ).forEach( tx -> tx.markForTermination( Status.Transaction.Terminated ) ); } 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 0af6242d1a0f9..94332b27d0d3d 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 @@ -73,7 +73,6 @@ import static org.neo4j.bolt.v1.messaging.util.MessageMatchers.msgSuccess; import static org.neo4j.bolt.v1.transport.integration.TransportTestUtil.eventuallyReceives; import static org.neo4j.helpers.collection.MapUtil.map; -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.ADMIN; @@ -459,7 +458,7 @@ private Map countTransactionsByUsername() neo.getLocalGraph().getDependencyResolver() ).stream() .filter( tx -> !tx.terminationReason().isPresent() ) - .map( tx -> getUsernameFromAccessMode( tx.mode() ) ) + .map( tx -> tx.mode().username() ) ).collect( Collectors.toMap( r -> r.username, r -> r.activeTransactions ) ); }