From 349e9fe81e380cce9bfce1121b12bcae2794b2d2 Mon Sep 17 00:00:00 2001 From: Olivia Ytterbrink Date: Wed, 31 Aug 2016 13:35:04 +0200 Subject: [PATCH] Fix termination of transactions in restricted access mode * Fix issue that we could miss terminating transactions while they were executing in a restricted access mode (e.g. procedure call, query planning) --- .../kernel/api/security/AuthSubject.java | 17 ++++++ .../api/security/OverriddenAccessMode.java | 22 ++++++++ .../security/auth/BasicAuthSubject.java | 6 +++ .../enterprise/auth/AuthProcedures.java | 11 ++-- .../auth/EnterpriseAuthSubject.java | 13 ++++- .../auth/AuthProceduresTestLogic.java | 46 ++++++++++++++++ .../enterprise/auth/AuthTestBase.java | 52 +++++++++++++++++++ 7 files changed, 161 insertions(+), 6 deletions(-) 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 9100dcc49d19..13971aa624a6 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 @@ -48,6 +48,11 @@ public interface AuthSubject extends AccessMode */ boolean allowsProcedureWith( String[] roleNames ) throws InvalidArgumentsException; + /** + * @return A string representing the primary principal of this subject + */ + String username(); + /** * Implementation to use when authentication has not yet been performed. Allows nothing. */ @@ -112,6 +117,12 @@ public String name() { return ""; } + + @Override + public String username() + { + return ""; // Should never clash with a valid username + } }; /** @@ -155,6 +166,12 @@ public String name() return ""; } + @Override + public String username() + { + return ""; // Should never clash with a valid username + } + @Override public void logout() { 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 03f244c657be..311127715e18 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,6 +21,7 @@ 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 { @@ -78,4 +79,25 @@ public String name() return originalMode.name() + " restricted to " + overriddenMode.name(); } } + + public String username() + { + return getUsernameFromAccessMode( originalMode ); + } + + public static String getUsernameFromAccessMode( AccessMode accessMode ) + { + if ( accessMode instanceof AuthSubject ) + { + return ((AuthSubject) accessMode).username(); + } + else if ( accessMode instanceof OverriddenAccessMode ) + { + return ((OverriddenAccessMode) accessMode).username(); + } + else + { + return ""; // Should never clash with a valid username + } + } } diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/BasicAuthSubject.java b/community/security/src/main/java/org/neo4j/server/security/auth/BasicAuthSubject.java index b5cb93e40f2e..f69e7c3ec4f4 100644 --- a/community/security/src/main/java/org/neo4j/server/security/auth/BasicAuthSubject.java +++ b/community/security/src/main/java/org/neo4j/server/security/auth/BasicAuthSubject.java @@ -147,6 +147,12 @@ public AuthorizationViolationException onViolation( String msg ) @Override public String name() + { + return username(); + } + + @Override + public String username() { return user.name(); } diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProcedures.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProcedures.java index 178a34230681..a2fa21d5464a 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProcedures.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProcedures.java @@ -41,6 +41,7 @@ import org.neo4j.procedure.Name; import org.neo4j.procedure.Procedure; +import static org.neo4j.kernel.impl.api.security.OverriddenAccessMode.getUsernameFromAccessMode; import static org.neo4j.procedure.Procedure.Mode.DBMS; public class AuthProcedures @@ -162,9 +163,9 @@ public Stream showCurrentUser() throws InvalidArgumentsException, IO { EnterpriseAuthSubject enterpriseSubject = EnterpriseAuthSubject.castOrFail( authSubject ); EnterpriseUserManager userManager = enterpriseSubject.getUserManager(); - return Stream.of( new UserResult( enterpriseSubject.name(), - userManager.getRoleNamesForUser( enterpriseSubject.name() ), - userManager.getUser( enterpriseSubject.name() ).getFlags() ) ); + return Stream.of( new UserResult( enterpriseSubject.username(), + userManager.getRoleNamesForUser( enterpriseSubject.username() ), + userManager.getUser( enterpriseSubject.username() ).getFlags() ) ); } @Procedure( name = "dbms.security.listUsers", mode = DBMS ) @@ -235,7 +236,7 @@ public Stream listTransactions() return countTransactionByUsername( getActiveTransactions().stream() .filter( tx -> !tx.terminationReason().isPresent() ) - .map( tx -> tx.mode().name() ) + .map( tx -> getUsernameFromAccessMode( tx.mode() ) ) ); } @@ -283,7 +284,7 @@ private Stream terminateTransactionsForValidUser( long terminatedCount = 0; for ( KernelTransactionHandle tx : getActiveTransactions() ) { - if ( tx.mode().name().equals( username ) && !tx.isUnderlyingTransaction( this.tx ) ) + if ( getUsernameFromAccessMode( tx.mode() ).equals( username ) && !tx.isUnderlyingTransaction( this.tx ) ) { boolean marked = tx.markForTermination( Status.Transaction.Terminated ); if ( marked ) diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/EnterpriseAuthSubject.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/EnterpriseAuthSubject.java index bafda5ae6da2..cce943ffce6c 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/EnterpriseAuthSubject.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/EnterpriseAuthSubject.java @@ -145,6 +145,17 @@ public AuthorizationViolationException onViolation( String msg ) @Override public String name() + { + String username = username(); + if ( username.isEmpty() ) + { + return ""; + } + return username; + } + + @Override + public String username() { Object principal = shiroSubject.getPrincipal(); if ( principal != null ) @@ -153,7 +164,7 @@ public String name() } else { - return ""; + return ""; // Should never clash with a valid username } } diff --git a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthProceduresTestLogic.java b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthProceduresTestLogic.java index f33c9d11d608..e9468465d4c4 100644 --- a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthProceduresTestLogic.java +++ b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthProceduresTestLogic.java @@ -30,6 +30,7 @@ import org.neo4j.bolt.v1.transport.socket.client.SocketConnection; import org.neo4j.helpers.HostnamePort; import org.neo4j.kernel.api.security.exception.InvalidArgumentsException; +import org.neo4j.test.DoubleLatch; import org.neo4j.test.rule.concurrent.ThreadingRule; import static java.lang.String.format; @@ -126,6 +127,28 @@ public void shouldListTransactions() throws Throwable write2.closeAndAssertSuccess(); } + @Test + public void shouldListRestrictedTransaction() + { + final DoubleLatch doubleLatch = new DoubleLatch( 2 ); + + ClassWithProcedures.setTestLatch( new ClassWithProcedures.LatchedRunnables( doubleLatch, () -> {}, () -> {} ) ); + + new Thread( () -> assertEmpty( writeSubject, "CALL test.waitForLatch()" ) ).start(); + doubleLatch.start(); + try + { + assertSuccess( adminSubject, "CALL dbms.security.listTransactions()", + r -> assertKeyIsMap( r, "username", "activeTransactions", + map( "adminSubject", "1", "writeSubject", "1" ) ) ); + } + finally + { + doubleLatch.finish(); + doubleLatch.awaitFinish(); + } + } + //---------- terminate transactions for user ----------- @Test @@ -260,6 +283,29 @@ public void shouldNotTerminateTransactionsIfNotAdmin() throws Throwable r -> assertKeyIs( r, "name", "writeSubject-node" ) ); } + @Test + public void shouldTerminateRestrictedTransaction() + { + final DoubleLatch doubleLatch = new DoubleLatch( 2 ); + + ClassWithProcedures.setTestLatch( new ClassWithProcedures.LatchedRunnables( doubleLatch, () -> {}, () -> {} ) ); + + new Thread( () -> assertFail( writeSubject, "CALL test.waitForLatch()", "Explicitly terminated by the user." ) ) + .start(); + + doubleLatch.start(); + try + { + assertSuccess( adminSubject, "CALL dbms.security.terminateTransactionsForUser( 'writeSubject' )", + r -> assertKeyIsMap( r, "username", "transactionsTerminated", map( "writeSubject", "1" ) ) ); + } + finally + { + doubleLatch.finish(); + doubleLatch.awaitFinish(); + } + } + //---------- change user password ----------- // Should change password for admin subject and valid user diff --git a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthTestBase.java b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthTestBase.java index 875f0dcf77e4..52d570df70ce 100644 --- a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthTestBase.java +++ b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthTestBase.java @@ -26,6 +26,7 @@ import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.stream.Stream; @@ -38,6 +39,7 @@ import org.neo4j.logging.Log; import org.neo4j.procedure.Context; import org.neo4j.procedure.Procedure; +import org.neo4j.test.DoubleLatch; import static java.util.stream.Collectors.toList; @@ -50,6 +52,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.neo4j.procedure.Procedure.Mode.READ; import static org.neo4j.procedure.Procedure.Mode.WRITE; import static org.neo4j.server.security.enterprise.auth.PredefinedRolesBuilder.ADMIN; import static org.neo4j.server.security.enterprise.auth.PredefinedRolesBuilder.ARCHITECT; @@ -423,6 +426,8 @@ public static class ClassWithProcedures @Context public Log log; + private static final AtomicReference testLatch = new AtomicReference<>(); + @Procedure( name = "test.numNodes" ) public Stream numNodes() { @@ -457,5 +462,52 @@ public void createNode() { db.createNode(); } + + @Procedure( name = "test.waitForLatch", mode = READ ) + public void waitForLatch() + { + try + { + testLatch.get().runBefore.run(); + } + finally + { + testLatch.get().doubleLatch.start(); + } + try + { + testLatch.get().runAfter.run(); + } + finally + { + testLatch.get().doubleLatch.finish(); + testLatch.get().doubleLatch.awaitFinish(); + } + } + + static class LatchedRunnables implements AutoCloseable + { + DoubleLatch doubleLatch; + Runnable runBefore; + Runnable runAfter; + + public LatchedRunnables( DoubleLatch doubleLatch, Runnable runBefore, Runnable runAfter ) + { + this.doubleLatch = doubleLatch; + this.runBefore = runBefore; + this.runAfter = runAfter; + } + + @Override + public void close() throws Exception + { + ClassWithProcedures.testLatch.set( null ); + } + } + + public static void setTestLatch( LatchedRunnables testLatch ) + { + ClassWithProcedures.testLatch.set( testLatch ); + } } }