Skip to content

Commit

Permalink
Add more tests
Browse files Browse the repository at this point in the history
Fix correct restriction of allowed procedures nested in regular procedures
- Change isElevated to isOverridden
- Add tests for nested user defined functions
- Add more complex tests for nested procedures
  • Loading branch information
henriknyman committed Oct 12, 2016
1 parent 0662879 commit 54a4e4a
Show file tree
Hide file tree
Showing 11 changed files with 191 additions and 52 deletions.
Expand Up @@ -589,9 +589,9 @@ final class TransactionBoundQueryContext(val transactionalContext: Transactional

private def shouldElevate(allowed: Array[String]): Boolean = {
// We have to be careful with elevation, since we cannot elevate permissions in a nested procedure call
// above the original allowed procedure mode. With enforce this by checking if mode is already an overridden mode.
// above the original allowed procedure mode. We enforce this by checking if mode is already an overridden mode.
val mode = transactionalContext.accessMode
allowed.nonEmpty && !mode.isElevated && mode.getOriginalAccessMode.allowsProcedureWith(allowed)
allowed.nonEmpty && !mode.isOverridden && mode.getOriginalAccessMode.allowsProcedureWith(allowed)
}

override def callReadOnlyProcedure(name: QualifiedName, args: Seq[Any], allowed: Array[String]) = {
Expand Down
Expand Up @@ -207,7 +207,7 @@ default AccessMode getOriginalAccessMode()
return this;
}

default boolean isElevated()
default boolean isOverridden()
{
return false;
}
Expand Down
Expand Up @@ -97,9 +97,9 @@ public boolean allowsSchemaWrites()
}

@Override
public boolean isElevated()
public boolean isOverridden()
{
return accessMode.isElevated();
return accessMode.isOverridden();
}

@Override
Expand Down
Expand Up @@ -47,12 +47,6 @@ public boolean allowsSchemaWrites()
return overriddenMode.allowsSchemaWrites() || originalMode.allowsSchemaWrites();
}

@Override
public boolean isElevated()
{
return true;
}

@Override
public String name()
{
Expand Down
Expand Up @@ -56,4 +56,10 @@ public AccessMode getSnapshot()
{
return AccessModeSnapshot.create( this );
}

@Override
public boolean isOverridden()
{
return true;
}
}
Expand Up @@ -71,9 +71,9 @@ public boolean allowsSchemaWrites()
}

@Override
public boolean isElevated()
public boolean isOverridden()
{
return AuthSubject.AUTH_DISABLED.isElevated();
return AuthSubject.AUTH_DISABLED.isOverridden();
}

@Override
Expand Down
Expand Up @@ -454,10 +454,9 @@ private long pollNumNodes()
}

/*
* Procedure 'test.allowedProcedure1' with READ mode and 'allowed = role1' is loaded.
* Procedure 'test.allowedProcedure2' with WRITE mode and 'allowed = role1' is loaded.
* Procedure 'test.allowedProcedure3' with SCHEMA mode and 'allowed = role1' is loaded.
* Procedure 'test.allowedProcedure4' with DBMS mode and 'allowed = role1' is loaded.
* Procedure 'test.allowedReadProcedure' with READ mode and 'allowed = role1' is loaded.
* Procedure 'test.allowedWriteProcedure' with WRITE mode and 'allowed = role1' is loaded.
* Procedure 'test.allowedSchemaProcedure' with SCHEMA mode and 'allowed = role1' is loaded.
* Admin creates a new user 'mats'.
* 'mats' logs in.
* 'mats' executes the procedures, access denied.
Expand Down
Expand Up @@ -45,6 +45,7 @@
import org.neo4j.procedure.Context;
import org.neo4j.procedure.Procedure;
import org.neo4j.procedure.TerminationGuard;
import org.neo4j.server.security.enterprise.auth.plugin.api.PredefinedRoles;
import org.neo4j.test.Barrier;
import org.neo4j.test.DoubleLatch;
import org.neo4j.test.rule.concurrent.ThreadingRule;
Expand Down Expand Up @@ -687,7 +688,7 @@ public void shouldHandleWriteAfterAllowedReadProcedureWithAuthDisabled() throws
.registerProcedure( ClassWithProcedures.class );

S subject = neo.login( "no_auth", "" );
assertEmpty( subject, "CALL test.allowedProcedure1() YIELD value CREATE (:NEWNODE {name:value})" );
assertEmpty( subject, "CALL test.allowedReadProcedure() YIELD value CREATE (:NEWNODE {name:value})" );
}

@Test
Expand All @@ -698,7 +699,7 @@ public void shouldHandleWriteAfterAllowedReadProcedureForWriteUser() throws Thro
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "role1Subject" );
userManager.addRoleToUser( PUBLISHER, "role1Subject" );
assertEmpty( neo.login( "role1Subject", "abc" ), "CALL test.allowedProcedure1() YIELD value CREATE (:NEWNODE {name:value})" );
assertEmpty( neo.login( "role1Subject", "abc" ), "CALL test.allowedReadProcedure() YIELD value CREATE (:NEWNODE {name:value})" );
}

@Test
Expand All @@ -709,11 +710,11 @@ public void shouldNotAllowNonWriterToWriteAfterCallingAllowedWriteProc() throws
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "nopermission" );
// should be able to invoke allowed procedure
assertSuccess( neo.login( "nopermission", "abc" ), "CALL test.allowedProcedure2()",
assertSuccess( neo.login( "nopermission", "abc" ), "CALL test.allowedWriteProcedure()",
itr -> assertEquals( itr.stream().collect( toList() ).size(), 2 ) );
// should not be able to do writes
assertFail( neo.login( "nopermission", "abc" ),
"CALL test.allowedProcedure2() YIELD value CREATE (:NEWNODE {name:value})", WRITE_OPS_NOT_ALLOWED );
"CALL test.allowedWriteProcedure() YIELD value CREATE (:NEWNODE {name:value})", WRITE_OPS_NOT_ALLOWED );
}

@Test
Expand All @@ -735,10 +736,10 @@ public void shouldNotAllowNonReaderToReadAfterCallingAllowedReadProc() throws Ex
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "nopermission" );
// should not be able to invoke any procedure
assertSuccess( neo.login( "nopermission", "abc" ), "CALL test.allowedProcedure1()",
assertSuccess( neo.login( "nopermission", "abc" ), "CALL test.allowedReadProcedure()",
itr -> assertEquals( itr.stream().collect( toList() ).size(), 1 ));
assertFail( neo.login( "nopermission", "abc" ),
"CALL test.allowedProcedure1() YIELD value MATCH (n:Secret) RETURN n.pass", READ_OPS_NOT_ALLOWED);
"CALL test.allowedReadProcedure() YIELD value MATCH (n:Secret) RETURN n.pass", READ_OPS_NOT_ALLOWED);
}

@Test
Expand All @@ -749,7 +750,7 @@ public void shouldHandleNestedReadProcedures() throws Throwable
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "role1Subject" );
assertSuccess( neo.login( "role1Subject", "abc" ),
"CALL test.nestedAllowedProcedure('test.allowedProcedure1') YIELD value",
"CALL test.nestedAllowedProcedure('test.allowedReadProcedure') YIELD value",
r -> assertKeyIs( r, "value", "foo" ) );
}

Expand All @@ -761,22 +762,101 @@ public void shouldHandleDoubleNestedReadProcedures() throws Throwable
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "role1Subject" );
assertSuccess( neo.login( "role1Subject", "abc" ),
"CALL test.nestedAllowedProcedure(\"test.nestedAllowedProcedure('test.allowedProcedure1')\") YIELD value",
"CALL test.doubleNestedAllowedProcedure YIELD value",
r -> assertKeyIs( r, "value", "foo" ) );
}

@Test
public void shouldFailNestedWriteProcedureFromReadProcedure() throws Throwable
public void shouldFailNestedAllowedWriteProcedureFromAllowedReadProcedure() throws Throwable
{
userManager = neo.getLocalUserManager();
userManager.newUser( "role1Subject", "abc", false );
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "role1Subject" );
assertFail( neo.login( "role1Subject", "abc" ),
"CALL test.nestedAllowedProcedure('test.allowedProcedure2') YIELD value",
"CALL test.nestedAllowedProcedure('test.allowedWriteProcedure') YIELD value",
WRITE_OPS_NOT_ALLOWED );
}

@Test
public void shouldRestrictNestedReadProcedureFromAllowedWriteProcedures() throws Throwable
{
userManager = neo.getLocalUserManager();
userManager.newUser( "role1Subject", "abc", false );
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "role1Subject" );
assertFail( neo.login( "role1Subject", "abc" ),
"CALL test.failingNestedAllowedWriteProcedure YIELD value",
WRITE_OPS_NOT_ALLOWED );
}

@Test
public void shouldHandleNestedReadProcedureWithDifferentAllowedRole() throws Throwable
{
userManager = neo.getLocalUserManager();
userManager.newUser( "role1Subject", "abc", false );
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "role1Subject" );
assertSuccess( neo.login( "role1Subject", "abc" ),
"CALL test.nestedAllowedProcedure('test.otherAllowedReadProcedure') YIELD value",
r -> assertKeyIs( r, "value", "foo" )
);
}

@Test
public void shouldFailNestedAllowedWriteProcedureFromNormalReadProcedure() throws Throwable
{
userManager = neo.getLocalUserManager();
userManager.newUser( "role1Subject", "abc", false );
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "role1Subject" );
userManager.addRoleToUser( PredefinedRoles.PUBLISHER, "role1Subject" ); // Even if subject has WRITE permission
// the procedure should restrict to READ
assertFail( neo.login( "role1Subject", "abc" ),
"CALL test.nestedReadProcedure('test.allowedWriteProcedure') YIELD value",
WRITE_OPS_NOT_ALLOWED );
}

@Test
public void shouldHandleFunctionWithAllowed() throws Throwable
{
userManager = neo.getLocalUserManager();

userManager.newUser( "role1Subject", "abc", false );
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "role1Subject" );
assertSuccess( neo.login( "role1Subject", "abc" ),
"RETURN test.allowedFunction1() AS value",
r -> assertKeyIs( r, "value", "foo" ) );
}

@Test
public void shouldHandleNestedFunctionsWithAllowed() throws Throwable
{
userManager = neo.getLocalUserManager();

userManager.newUser( "role1Subject", "abc", false );
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "role1Subject" );
assertSuccess( neo.login( "role1Subject", "abc" ),
"RETURN test.nestedAllowedFunction('test.allowedFunction1()') AS value",
r -> assertKeyIs( r, "value", "foo" ) );
}

@Test
public void shouldHandleNestedFunctionWithDifferentAllowedRole() throws Throwable
{
userManager = neo.getLocalUserManager();

userManager.newUser( "role1Subject", "abc", false );
userManager.newRole( "role1" );
userManager.addRoleToUser( "role1", "role1Subject" );
assertSuccess( neo.login( "role1Subject", "abc" ),
"RETURN test.nestedAllowedFunction('test.allowedFunction2()') AS value",
r -> assertKeyIs( r, "value", "foo" )
);
}

/*
This surface is hidden in 3.1, to possibly be completely removed or reworked later
==================================================================================
Expand Down
Expand Up @@ -117,9 +117,9 @@ public boolean allowsSchemaWrites()
}

@Override
public boolean isElevated()
public boolean isOverridden()
{
return ANONYMOUS.isElevated();
return ANONYMOUS.isOverridden();
}

@Override
Expand Down

0 comments on commit 54a4e4a

Please sign in to comment.