Skip to content

Commit

Permalink
Fix issues with change password procedure
Browse files Browse the repository at this point in the history
- Add Bolt integration tests for the new built-in change password procedure
- Fix access mode for beginImplicitTransaction in Bolt session state machine
- Expose restrict access mode on Neo4jTransactionContext
- Change access mode to READ during query planning
- Make tokenNameLookup a lazy val in TransactionBoundQueryContext
to be able to execute queries without having read access
  • Loading branch information
henriknyman committed Mar 10, 2016
1 parent df29bce commit 320c8a4
Show file tree
Hide file tree
Showing 17 changed files with 131 additions and 16 deletions.
Expand Up @@ -79,7 +79,9 @@ private AccessMode authenticate( String user, String password ) throws Authentic
case SUCCESS:
break;
case PASSWORD_CHANGE_REQUIRED:
throw new AuthenticationException( Status.Security.CredentialsExpired, identifier.get() );
// TODO: We just return OK for now, but we should notify the client with an appropriate message
//throw new AuthenticationException( Status.Security.CredentialsExpired, identifier.get() );
break;
case TOO_MANY_ATTEMPTS:
throw new AuthenticationException( Status.Security.AuthenticationRateLimit, identifier.get() );
default:
Expand Down
Expand Up @@ -132,7 +132,7 @@ public State beginImplicitTransaction( SessionStateMachine ctx )
// NOTE: If we move away from doing implicit transactions this
// way, we need a different way to kill statements running in implicit
// transactions, because we do that by calling #terminate() on this tx.
ctx.currentTransaction = ctx.spi.beginTransaction( implicit, AccessMode.Static.FULL );
ctx.currentTransaction = ctx.spi.beginTransaction( implicit, ctx.accessMode );
return IN_TRANSACTION;
}

Expand Down
Expand Up @@ -27,6 +27,7 @@
import org.junit.runners.Parameterized;

import java.util.Collection;
import java.util.Collections;

import org.neo4j.bolt.v1.transport.socket.client.Connection;
import org.neo4j.bolt.v1.transport.socket.client.SecureSocketConnection;
Expand All @@ -41,6 +42,8 @@
import static java.util.Arrays.asList;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.neo4j.bolt.v1.messaging.message.Messages.init;
import static org.neo4j.bolt.v1.messaging.message.Messages.pullAll;
import static org.neo4j.bolt.v1.messaging.message.Messages.run;
import static org.neo4j.bolt.v1.messaging.util.MessageMatchers.msgFailure;
import static org.neo4j.bolt.v1.messaging.util.MessageMatchers.msgSuccess;
import static org.neo4j.bolt.v1.transport.integration.TransportTestUtil.eventuallyRecieves;
Expand Down Expand Up @@ -97,8 +100,10 @@ public void shouldRespondWithCredentialsExpiredOnFirstUse() throws Throwable

// Then
assertThat( client, eventuallyRecieves( new byte[]{0, 0, 0, 1} ) );
assertThat( client, eventuallyRecieves( msgFailure( Status.Security.CredentialsExpired,
String.format( "The credentials have expired and need to be updated. (ID:%s)", server.uniqueIdentier() ) ) ) );
// TODO: The server will just return OK for now, but this should be changed to an appropriate message
//assertThat( client, eventuallyRecieves( msgFailure( Status.Security.CredentialsExpired,
// String.format( "The credentials have expired and need to be updated. (ID:%s)", server.uniqueIdentier() ) ) ) );
assertThat( client, eventuallyRecieves( msgSuccess() ) );
}

@Test
Expand Down Expand Up @@ -154,6 +159,81 @@ public void shouldBeAbleToUpdateCredentials() throws Throwable
assertThat( client, eventuallyRecieves( msgSuccess() ) );
}

@Test
public void shouldBeAbleToChangePasswordUsingBuiltInProcedure() throws Throwable
{
// When
client.connect( address )
.send( TransportTestUtil.acceptedVersions( 1, 0, 0, 0 ) )
.send( TransportTestUtil.chunk(
init( "TestClient/1.1",
map( "principal", "neo4j", "credentials", "neo4j", "scheme", "basic" ) ) ) );


// Then
assertThat( client, eventuallyRecieves( new byte[]{0, 0, 0, 1} ) );
// TODO: For now the server just returns OK when a password change is required, but this should be changed to an appropriate message
//assertThat( client, eventuallyRecieves( msgFailure( Status.Security.CredentialsExpired,
// "The credentials have expired and need to be updated." ) ) );
assertThat( client, eventuallyRecieves( msgSuccess() ) );

// When
client.send( TransportTestUtil.chunk(
run( "CALL sys.auth.changePassword", Collections.singletonMap( "password", "secret" ) ),
pullAll() ) );

// Then
assertThat( client, eventuallyRecieves( msgSuccess() ) );

// If I reconnect I cannot use the old password
reconnect();
client.connect( address )
.send( TransportTestUtil.acceptedVersions( 1, 0, 0, 0 ) )
.send( TransportTestUtil.chunk(
init( "TestClient/1.1",
map( "principal", "neo4j", "credentials", "neo4j", "scheme", "basic" ) ) ) );
assertThat( client, eventuallyRecieves( new byte[]{0, 0, 0, 1} ) );
assertThat( client, eventuallyRecieves( msgFailure( Status.Security.Unauthorized,
String.format( "The client is unauthorized due to authentication failure. (ID:%s)", server.uniqueIdentier()) ) ) );

// But the new password works fine
reconnect();
client.connect( address )
.send( TransportTestUtil.acceptedVersions( 1, 0, 0, 0 ) )
.send( TransportTestUtil.chunk(
init( "TestClient/1.1",
map( "principal", "neo4j", "credentials", "secret", "scheme", "basic" ) ) ) );
assertThat( client, eventuallyRecieves( new byte[]{0, 0, 0, 1} ) );
assertThat( client, eventuallyRecieves( msgSuccess() ) );
}

@Test
public void shouldNotBeAbleToReadWhenPasswordChangeRequired() throws Throwable
{
// When
client.connect( address )
.send( TransportTestUtil.acceptedVersions( 1, 0, 0, 0 ) )
.send( TransportTestUtil.chunk(
init( "TestClient/1.1",
map( "principal", "neo4j", "credentials", "neo4j", "scheme", "basic" ) ) ) );


// Then
assertThat( client, eventuallyRecieves( new byte[]{0, 0, 0, 1} ) );
// TODO: For now the server just returns OK when a password change is required, but this should be changed to an appropriate message
//assertThat( client, eventuallyRecieves( msgFailure( Status.Security.CredentialsExpired,
// "The credentials have expired and need to be updated." ) ) );
assertThat( client, eventuallyRecieves( msgSuccess() ) );

// When
client.send( TransportTestUtil.chunk(
run( "MATCH (n) RETURN n" ),
pullAll() ) );

// Then
assertThat( client, eventuallyRecieves( msgFailure( Status.Security.Forbidden,
"Read operations are not allowed for `NONE` transactions." ) ) );
}

@Before
public void setup()
Expand Down
Expand Up @@ -31,6 +31,7 @@ import org.neo4j.cypher.internal.tracing.{CompilationTracer, TimingCompilationTr
import org.neo4j.graphdb.config.Setting
import org.neo4j.graphdb.factory.GraphDatabaseSettings
import org.neo4j.kernel.api.ReadOperations
import org.neo4j.kernel.api.security.AccessMode
import org.neo4j.kernel.configuration.Config
import org.neo4j.kernel.impl.core.ThreadToStatementContextBridge
import org.neo4j.kernel.impl.query.{QueryExecutionMonitor, QuerySession, TransactionalContext}
Expand Down Expand Up @@ -136,6 +137,9 @@ class ExecutionEngine(val queryService: GraphDatabaseQueryService, logProvider:
// create transaction and query context
val tc = externalTransactionalContext.provideContext()

// Temporarily change access mode during query planning
val revertable = tc.restrict(AccessMode.Static.READ)

val ((plan: ExecutionPlan, extractedParameters), touched) = try {
// fetch plan cache
val cache = getOrCreateFromSchemaState(tc.readOperations, {
Expand All @@ -156,6 +160,8 @@ class ExecutionEngine(val queryService: GraphDatabaseQueryService, logProvider:
case (t: Throwable) =>
tc.close(success = false)
throw t
} finally {
revertable.close()
}

if (touched) {
Expand Down
Expand Up @@ -25,7 +25,7 @@ import java.{lang, util}
import org.neo4j.cypher._
import org.neo4j.cypher.internal._
import org.neo4j.cypher.internal.compiler.v3_0
import org.neo4j.cypher.internal.compiler.v3_0.executionplan.{ExecutionPlan => ExecutionPlan_v3_0, InternalExecutionResult, READ_ONLY, READ_WRITE, SCHEMA_WRITE, WRITE}
import org.neo4j.cypher.internal.compiler.v3_0.executionplan.{ExecutionPlan => ExecutionPlan_v3_0, _}
import org.neo4j.cypher.internal.compiler.v3_0.planDescription.InternalPlanDescription.Arguments._
import org.neo4j.cypher.internal.compiler.v3_0.planDescription.{Argument, InternalPlanDescription, PlanDescriptionArgumentSerializer}
import org.neo4j.cypher.internal.compiler.v3_0.spi.{InternalResultRow, InternalResultVisitor}
Expand Down Expand Up @@ -332,6 +332,7 @@ case class ExecutionResultWrapperFor3_0(inner: InternalExecutionResult, planner:
case READ_WRITE => QueryExecutionType.QueryType.READ_WRITE
case WRITE => QueryExecutionType.QueryType.WRITE
case SCHEMA_WRITE => QueryExecutionType.QueryType.SCHEMA_WRITE
case DBMS => QueryExecutionType.QueryType.READ_ONLY // TODO: We need to decide how we expose this in the public API
}
inner.executionMode match {
case ExplainModev3_0 => QueryExecutionType.explained(qt)
Expand Down
Expand Up @@ -22,6 +22,8 @@ package org.neo4j.cypher.internal.spi
import org.neo4j.cypher.internal.compiler.v3_0.spi.QueryTransactionalContext
import org.neo4j.graphdb.{Lock, PropertyContainer}
import org.neo4j.kernel.GraphDatabaseQueryService
import org.neo4j.kernel.api.KernelTransaction.Revertable
import org.neo4j.kernel.api.security.AccessMode
import org.neo4j.kernel.api.txstate.TxStateHolder
import org.neo4j.kernel.api.{ReadOperations, Statement}
import org.neo4j.kernel.impl.query.TransactionalContext
Expand Down Expand Up @@ -52,4 +54,6 @@ case class TransactionalContextWrapper(tc: TransactionalContext) extends QueryTr
override def isTopLevelTx: Boolean = tc.isTopLevelTx

override def close(success: Boolean) { tc.close(success) }

def restrict(accessMode: AccessMode): Revertable = tc.restrict(accessMode)
}
Expand Up @@ -140,6 +140,7 @@ class TransactionBoundPlanContext(tc: TransactionalContextWrapper)
private def asCypherProcMode(mode: KernelProcedureSignature.Mode): ProcedureCallMode = mode match {
case KernelProcedureSignature.Mode.READ_ONLY => LazyReadOnlyCallMode
case KernelProcedureSignature.Mode.READ_WRITE => EagerReadWriteCallMode
case KernelProcedureSignature.Mode.DBMS => DbmsCallMode
case _ => throw new CypherExecutionException(
"Unable to execute procedure, because it requires an unrecognized execution mode: " + mode.name(), null )
}
Expand Down
Expand Up @@ -481,7 +481,7 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional

override def relationshipEndNode(rel: Relationship) = rel.getEndNode

private val tokenNameLookup = new StatementTokenNameLookup(transactionalContext.statement.readOperations())
private lazy val tokenNameLookup = new StatementTokenNameLookup(transactionalContext.statement.readOperations())

// Legacy dependency between kernel and compiler
override def variableLengthPathExpand(node: PatternNode,
Expand Down
Expand Up @@ -41,6 +41,7 @@ import org.neo4j.cypher.internal.spi.v3_0.MonoDirectionalTraversalMatcher
import org.neo4j.cypher.internal.{ExecutionEngine, ExecutionPlan, ExecutionResult}
import org.neo4j.cypher.javacompat.internal.GraphDatabaseCypherService
import org.neo4j.graphdb._
import org.neo4j.kernel.api.KernelTransaction.Revertable
import org.neo4j.kernel.api.security.AccessMode
import org.neo4j.kernel.api.{KernelTransaction, ReadOperations, Statement}
import org.neo4j.kernel.configuration.Config
Expand Down Expand Up @@ -244,6 +245,8 @@ class LazyTest extends ExecutionEngineFunSuite {

val tx = fakeGraph.beginTransaction(KernelTransaction.Type.`implicit`, AccessMode.Static.FULL)
val context = new Neo4jTransactionalContext(service, tx, fakeStatement, new PropertyContainerLocker)
val revertableRestrict = mock[Revertable]
when(context.restrict(anyObject())).thenReturn(revertableRestrict)
val session = QueryEngineProvider.embeddedSession(context)

//When:
Expand Down
Expand Up @@ -151,7 +151,7 @@ interface CloseListener
*/
Type transactionType();

Revertable restrict( AccessMode read );
Revertable restrict( AccessMode mode );

interface Revertable extends AutoCloseable
{
Expand Down
Expand Up @@ -20,26 +20,21 @@
package org.neo4j.kernel.builtinprocs;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;

import org.neo4j.collection.RawIterator;
import org.neo4j.graphdb.Label;
import org.neo4j.graphdb.security.AuthorizationViolationException;
import org.neo4j.kernel.api.exceptions.ProcedureException;
import org.neo4j.kernel.api.exceptions.Status;
import org.neo4j.kernel.api.proc.Neo4jTypes;

import static org.neo4j.kernel.api.proc.Neo4jTypes.NTString;

import org.neo4j.kernel.api.proc.CallableProcedure;
import org.neo4j.kernel.api.proc.ProcedureSignature;
import org.neo4j.kernel.api.proc.ProcedureSignature.ProcedureName;
import org.neo4j.kernel.api.security.AccessMode;
import org.neo4j.kernel.impl.api.TokenAccess;
import org.neo4j.server.security.auth.AuthSubject;

import static org.neo4j.helpers.collection.Iterators.asRawIterator;
import static org.neo4j.helpers.collection.Iterators.map;
import static org.neo4j.kernel.api.proc.Neo4jTypes.NTString;
import static org.neo4j.kernel.api.proc.ProcedureSignature.procedureSignature;

/**
Expand Down Expand Up @@ -69,7 +64,7 @@ public RawIterator<Object[],ProcedureException> apply( Context ctx, Object[] inp
try
{
boolean result = authSubject.setPassword( input[0].toString() );
return map( ( l ) -> new Object[]{l}, asRawIterator( Arrays.asList( result ).iterator() ) );
return map( ( l ) -> new Object[]{l}, asRawIterator( Collections.singletonList( result ).iterator() ) );
}
catch ( IOException e )
{
Expand Down
Expand Up @@ -27,4 +27,5 @@ public interface InternalTransaction extends Transaction
{
KernelTransaction.Type transactionType();
AccessMode mode();
KernelTransaction.Revertable restrict( AccessMode mode );
}
Expand Up @@ -90,4 +90,10 @@ public AccessMode mode()
{
return currentTransaction.get().mode();
}

@Override
public KernelTransaction.Revertable restrict( AccessMode mode )
{
return currentTransaction.get().restrict( mode );
}
}
Expand Up @@ -130,4 +130,10 @@ public AccessMode mode()
{
return transaction.mode();
}

@Override
public KernelTransaction.Revertable restrict( AccessMode mode )
{
return transaction.restrict( mode );
}
}
Expand Up @@ -161,4 +161,10 @@ public Lock acquireWriteLock( PropertyContainer p )
{
return locker.exclusiveLock( () -> statement, p );
}

@Override
public KernelTransaction.Revertable restrict( AccessMode accessMode )
{
return transaction.restrict( accessMode );
}
}
Expand Up @@ -22,8 +22,10 @@
import org.neo4j.graphdb.Lock;
import org.neo4j.graphdb.PropertyContainer;
import org.neo4j.kernel.GraphDatabaseQueryService;
import org.neo4j.kernel.api.KernelTransaction;
import org.neo4j.kernel.api.ReadOperations;
import org.neo4j.kernel.api.Statement;
import org.neo4j.kernel.api.security.AccessMode;
import org.neo4j.kernel.api.txstate.TxStateHolder;

public interface TransactionalContext
Expand Down Expand Up @@ -52,4 +54,6 @@ public interface TransactionalContext

QuerySession.MetadataKey<TransactionalContext>
metadataKey = new QuerySession.MetadataKey<>( TransactionalContext.class, "transaction context" );

KernelTransaction.Revertable restrict( AccessMode accessMode );
}
Expand Up @@ -91,6 +91,6 @@ public boolean allowsSchemaWrites()
@Override
public String name()
{
return null;
return accessMode.name();
}
}

0 comments on commit 320c8a4

Please sign in to comment.