Freeze security context before acquiring transaction, to avoid tx lea…

…kage
neo4j · Nov 24, 2016 · 0ffa478 · 0ffa478
1 parent 1e1bac5
commit 0ffa478
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 22 deletions.
diff --git a/...unity/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactionImplementation.java b/...unity/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactionImplementation.java
@@ -215,8 +215,8 @@ public KernelTransactionImplementation( StatementOperationContainer operationCon
      * Reset this transaction to a vanilla state, turning it into a logically new transaction.
      */
     public KernelTransactionImplementation initialize(
-            long lastCommittedTx, long lastTimeStamp, StatementLocks statementLocks, Type type, SecurityContext securityContext,
-            long transactionTimeout )
+            long lastCommittedTx, long lastTimeStamp, StatementLocks statementLocks, Type type,
+            SecurityContext frozenSecurityContext, long transactionTimeout )
     {
         this.type = type;
         this.statementLocks = statementLocks;
@@ -229,7 +229,7 @@ public KernelTransactionImplementation initialize(
         this.lastTransactionTimestampWhenStarted = lastTimeStamp;
         this.transactionEvent = tracer.beginTransaction();
         assert transactionEvent != null : "transactionEvent was null!";
-        this.securityContext = securityContext.freeze();
+        this.securityContext = frozenSecurityContext;
         this.transactionId = NOT_COMMITTED_TRANSACTION_ID;
         this.commitTime = NOT_COMMITTED_TRANSACTION_COMMIT_TIME;
         this.currentTransactionOperations = timeoutMillis > 0 ? operationContainer.guardedParts() : operationContainer.nonGuarderParts();

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactions.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactions.java
@@ -157,6 +157,7 @@ public KernelTransactionImplementation newInstance()
     public KernelTransaction newInstance( KernelTransaction.Type type, SecurityContext securityContext, long timeout )
     {
         assertCurrentThreadIsNotBlockingNewTransactions();
+        SecurityContext frozenSecurityContext = securityContext.freeze();
         newTransactionsLock.readLock().lock();
         try
         {
@@ -165,7 +166,7 @@ public KernelTransaction newInstance( KernelTransaction.Type type, SecurityConte
             KernelTransactionImplementation tx = localTxPool.acquire();
             StatementLocks statementLocks = statementLocksFactory.newInstance();
             tx.initialize( lastCommittedTransaction.transactionId(),
-                    lastCommittedTransaction.commitTimestamp(), statementLocks, type, securityContext, timeout );
+                    lastCommittedTransaction.commitTimestamp(), statementLocks, type, frozenSecurityContext, timeout );
             return tx;
         }
         finally
@@ -297,5 +298,4 @@ private void assertCurrentThreadIsNotBlockingNewTransactions()
                     "Thread that is blocking new transactions from starting can't start new transaction" );
         }
     }
-
 }
diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/KernelTransactionsTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/KernelTransactionsTest.java
@@ -32,11 +32,13 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReferenceArray;
 
+import org.neo4j.graphdb.security.AuthorizationExpiredException;
 import org.neo4j.kernel.api.KernelTransaction;
 import org.neo4j.kernel.api.KernelTransactionHandle;
 import org.neo4j.kernel.api.exceptions.Status;
 import org.neo4j.kernel.api.exceptions.TransactionFailureException;
 import org.neo4j.kernel.api.security.AnonymousContext;
+import org.neo4j.kernel.api.security.SecurityContext;
 import org.neo4j.kernel.impl.api.state.ConstraintIndexCreator;
 import org.neo4j.kernel.impl.factory.AccessCapability;
 import org.neo4j.kernel.impl.factory.CanWrite;
@@ -69,9 +71,8 @@
 
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.locks.LockSupport.parkNanos;
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.hamcrest.CoreMatchers.instanceOf;
-import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.CoreMatchers.*;
+import static org.hamcrest.Matchers.empty;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -90,6 +91,7 @@
 import static org.neo4j.kernel.api.KernelTransaction.Type.explicit;
 import static org.neo4j.kernel.api.security.SecurityContext.AUTH_DISABLED;
 import static org.neo4j.kernel.impl.transaction.TransactionHeaderInformationFactory.DEFAULT;
+import static org.neo4j.test.assertion.Assert.assertException;
 
 public class KernelTransactionsTest
 {
@@ -374,7 +376,19 @@ public void unblockNewTransactionsFromWrongThreadThrows() throws Exception
         assertNotNull( txOpener.get( 2, TimeUnit.SECONDS ) );
     }
 
-    private static void startAndCloseTransaction( KernelTransactions kernelTransactions )
+    @Test
+    public void shouldNotLeakTransactionOnSecurityContextFreezeFailure() throws Exception {
+        KernelTransactions kernelTransactions = newKernelTransactions();
+        SecurityContext securityContext = mock(SecurityContext.class);
+        when(securityContext.freeze()).thenThrow(new AuthorizationExpiredException("Freeze failed."));
+
+        assertException(() -> kernelTransactions.newInstance(KernelTransaction.Type.explicit, securityContext, 0L),
+                AuthorizationExpiredException.class, "Freeze failed.");
+
+        assertThat("We should not have any transaction", kernelTransactions.activeTransactions(), is(empty()));
+    }
+
+    private static void startAndCloseTransaction(KernelTransactions kernelTransactions)
     {
         try
         {
@@ -490,25 +504,21 @@ private static KernelTransactionHandle newHandle( KernelTransaction tx )
         return new TestKernelTransactionHandle( tx );
     }
 
-    protected KernelTransaction getKernelTransaction( KernelTransactions transactions )
+    private KernelTransaction getKernelTransaction( KernelTransactions transactions )
     {
         return transactions.newInstance( KernelTransaction.Type.implicit, AnonymousContext.none(), 0L );
     }
 
     private static class TestKernelTransactions extends KernelTransactions
     {
-        public TestKernelTransactions( StatementLocksFactory statementLocksFactory,
-                                       ConstraintIndexCreator constraintIndexCreator,
-                                       StatementOperationContainer statementOperationsContianer,
-                                       SchemaWriteGuard schemaWriteGuard,
-                                       TransactionHeaderInformationFactory txHeaderFactory,
-                                       TransactionCommitProcess transactionCommitProcess,
-                                       IndexConfigStore indexConfigStore,
-                                       LegacyIndexProviderLookup legacyIndexProviderLookup, TransactionHooks hooks,
-                                       TransactionMonitor transactionMonitor, LifeSupport dataSourceLife,
-                                       Tracers tracers, StorageEngine storageEngine, Procedures procedures,
-                                       TransactionIdStore transactionIdStore, Clock clock,
-                                       AccessCapability accessCapability )
+        TestKernelTransactions( StatementLocksFactory statementLocksFactory,
+                ConstraintIndexCreator constraintIndexCreator, StatementOperationContainer statementOperationsContianer,
+                SchemaWriteGuard schemaWriteGuard, TransactionHeaderInformationFactory txHeaderFactory,
+                TransactionCommitProcess transactionCommitProcess, IndexConfigStore indexConfigStore,
+                LegacyIndexProviderLookup legacyIndexProviderLookup, TransactionHooks hooks,
+                TransactionMonitor transactionMonitor, LifeSupport dataSourceLife, Tracers tracers,
+                StorageEngine storageEngine, Procedures procedures, TransactionIdStore transactionIdStore, Clock clock,
+                AccessCapability accessCapability )
         {
             super( statementLocksFactory, constraintIndexCreator, statementOperationsContianer, schemaWriteGuard,
                     txHeaderFactory, transactionCommitProcess, indexConfigStore, legacyIndexProviderLookup, hooks,