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)

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 "<anonymous>";
         }
+
+        @Override
+        public String username()
+        {
+            return ""; // Should never clash with a valid username
+        }
     };
 
     /**
@@ -155,6 +166,12 @@ public String name()
             return "<auth disabled>";
         }
 
+        @Override
+        public String username()
+        {
+            return ""; // Should never clash with a valid username
+        }
+
         @Override
         public void logout()
         {

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
+        }
+    }
 }

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();
     }

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<UserResult> 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<TransactionResult> 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<TransactionTerminationResult> 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 )

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 "<missing_principal>";
+        }
+        return username;
+    }
+
+    @Override
+    public String username()
     {
         Object principal = shiroSubject.getPrincipal();
         if ( principal != null )
@@ -153,7 +164,7 @@ public String name()
         }
         else
         {
-            return "<missing_principal>";
+            return ""; // Should never clash with a valid username
         }
     }
 

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

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<LatchedRunnables> testLatch = new AtomicReference<>();
+
         @Procedure( name = "test.numNodes" )
         public Stream<CountResult> 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 );
+        }
     }
 }