Skip to content

Commit

Permalink
Generalized procedure warnings
Browse files Browse the repository at this point in the history
  • Loading branch information
OliviaYtterbrink authored and eebus committed Jan 31, 2017
1 parent 57d2527 commit 99b3643
Show file tree
Hide file tree
Showing 14 changed files with 73 additions and 41 deletions.
Expand Up @@ -389,7 +389,8 @@ enum Procedure implements Status
"A procedure is using or receiving a value of an invalid type." ),
ProcedureTimedOut( ClientError,
"The procedure has not completed within the specified timeout. You may want to retry with a longer " +
"timeout." );
"timeout." ),
ProcedureWarning( ClientNotification, "The query used a procedure that generate a warning." );

private final Code code;

Expand All @@ -416,8 +417,7 @@ enum Security implements Status
Forbidden( ClientError, "An attempt was made to perform an unauthorized action." ),
AuthorizationExpired( ClientError, "The stored authorization info has expired. Please reconnect." ),
AuthProviderTimeout( TransientError, "An auth provider request timed out." ),
AuthProviderFailed( TransientError, "An auth provider request failed." ),
NativeProcedureWarning( ClientNotification, "Native user management procedures will not affect non-native users." );
AuthProviderFailed( TransientError, "An auth provider request failed." );

private final Code code;

Expand Down
Expand Up @@ -75,8 +75,8 @@ object ProcedureWarnings extends VisitorPhase {

private def findWarnings(statement: Statement): Set[InternalNotification] =
statement.treeFold(Set.empty[InternalNotification]) {
case f@ResolvedCall(ProcedureSignature(name, _, _, _, _, _, true), _, _, _, _) =>
(seq) => (seq + ProcedureWarningNotification(f.position, name.toString), None)
case f@ResolvedCall(ProcedureSignature(name, _, _, _, _, _, Some(warning)), _, _, _, _) =>
(seq) => (seq + ProcedureWarningNotification(f.position, name.toString, warning), None)
case _:UnresolvedCall =>
throw new InternalException("Expected procedures to have been resolved already")
}
Expand Down
Expand Up @@ -28,13 +28,13 @@ case class ProcedureSignature(name: QualifiedName,
deprecationInfo: Option[String],
accessMode: ProcedureAccessMode,
description: Option[String] = None,
warning: Boolean = false) {
warning: Option[String] = None) {

def outputFields = outputSignature.getOrElse(Seq.empty)

def isVoid = outputSignature.isEmpty

override def toString = {0
override def toString = {
val sig = inputSignature.mkString(", ")
outputSignature.map(out => s"$name($sig) :: ${out.mkString(", ")}").getOrElse(s"$name($sig) :: VOID")
}
Expand Down
Expand Up @@ -145,8 +145,8 @@ class ExecutionResultWrapper(val inner: InternalExecutionResult, val planner: Pl
NotificationCode.DEPRECATED_PROCEDURE.notification(pos.asInputPosition, NotificationDetail.Factory.deprecatedName(oldName, newName))
case DeprecatedPlannerNotification =>
NotificationCode.DEPRECATED_PLANNER.notification(graphdb.InputPosition.empty)
case ProcedureWarningNotification(pos, name) =>
NotificationCode.PROCEDURE_WARNING.notification(pos.asInputPosition)
case ProcedureWarningNotification(pos, name, warning) =>
NotificationCode.PROCEDURE_WARNING.notification(pos.asInputPosition, NotificationDetail.Factory.procedureWarning(name, warning))
}

override def accept[EX <: Exception](visitor: ResultVisitor[EX]) = inner.accept(wrapVisitor(visitor))
Expand Down
Expand Up @@ -137,7 +137,7 @@ class TransactionBoundPlanContext(tc: TransactionalContextWrapper, logger: Inter
val deprecationInfo = asOption(ks.deprecated())
val mode = asCypherProcMode(ks.mode(), ks.allowed())
val description = asOption(ks.description())
val warning = ks.warning()
val warning = asOption(ks.warning())

ProcedureSignature(name, input, output, deprecationInfo, mode, description, warning)
}
Expand Down
Expand Up @@ -674,7 +674,7 @@ class ExecutionEngineIT extends CypherFunSuite with GraphIcing {
val emptySignature = List.empty[FieldSignature].asJava
val signature: ProcedureSignature = new ProcedureSignature(
procedureName, paramSignature, resultSignature, Mode.READ, java.util.Optional.empty(), Array.empty,
java.util.Optional.empty(), false)
java.util.Optional.empty(), java.util.Optional.empty())

def paramSignature = List.empty[FieldSignature].asJava

Expand Down
Expand Up @@ -60,6 +60,6 @@ case class DeprecatedFunctionNotification(position: InputPosition, oldName: Stri

case class DeprecatedProcedureNotification(position: InputPosition, oldName: String, newName: String) extends InternalNotification

case class ProcedureWarningNotification(position: InputPosition, procedure: String) extends InternalNotification
case class ProcedureWarningNotification(position: InputPosition, procedure: String, warning: String) extends InternalNotification

case object DeprecatedPlannerNotification extends InternalNotification
Expand Up @@ -103,8 +103,8 @@ public enum NotificationCode
),
PROCEDURE_WARNING(
SeverityLevel.WARNING,
Status.Security.NativeProcedureWarning,
"Native user management procedures will not affect non-native users."
Status.Procedure.ProcedureWarning,
"The query used a procedure that generate a warning."
),
EAGER_LOAD_CSV(
SeverityLevel.WARNING,
Expand Down
Expand Up @@ -51,6 +51,11 @@ public static NotificationDetail relationshipType( final String relType )
return createNotificationDetail( "the missing relationship type is", relType, true );
}

public static NotificationDetail procedureWarning( final String procedure, final String warning )
{
return createProcedureWarningNotificationDetail( procedure, warning );
}

public static NotificationDetail propertyName( final String name )
{
return createNotificationDetail( "the missing property name is", name, true );
Expand Down Expand Up @@ -162,5 +167,29 @@ public String toString()
}
};
}

private static NotificationDetail createProcedureWarningNotificationDetail( String procedure, String warning )
{
return new NotificationDetail()
{
@Override
public String name()
{
return procedure;
}

@Override
public String value()
{
return warning;
}

@Override
public String toString()
{
return String.format( warning, procedure );
}
};
}
}
}
Expand Up @@ -46,7 +46,7 @@ public class ProcedureSignature
private final Optional<String> deprecated;
private final String[] allowed;
private final Optional<String> description;
private final boolean warn;
private final Optional<String> warning;

public ProcedureSignature( QualifiedName name,
List<FieldSignature> inputSignature,
Expand All @@ -55,7 +55,7 @@ public ProcedureSignature( QualifiedName name,
Optional<String> deprecated,
String[] allowed,
Optional<String> description,
boolean warn)
Optional<String> warning)
{
this.name = name;
this.inputSignature = unmodifiableList( inputSignature );
Expand All @@ -64,7 +64,7 @@ public ProcedureSignature( QualifiedName name,
this.deprecated = deprecated;
this.allowed = allowed;
this.description = description;
this.warn = warn;
this.warning = warning;
}

public QualifiedName name()
Expand Down Expand Up @@ -101,9 +101,9 @@ public Optional<String> description()
return description;
}

public boolean warning()
public Optional<String> warning()
{
return warn;
return warning;
}

@Override
Expand Down Expand Up @@ -151,7 +151,7 @@ public static class Builder
private Optional<String> deprecated = Optional.empty();
private String[] allowed = new String[0];
private Optional<String> description = Optional.empty();
private boolean warn = false;
private Optional<String> warning = Optional.empty();

public Builder( String[] namespace, String name )
{
Expand Down Expand Up @@ -202,15 +202,15 @@ public Builder allowed( String[] allowed )
return this;
}

public Builder warning( boolean warn )
public Builder warning( String warning )
{
this.warn = warn;
this.warning = Optional.of( warning );
return this;
}

public ProcedureSignature build()
{
return new ProcedureSignature(name, inputSignature, outputSignature, mode, deprecated, allowed, description, warn );
return new ProcedureSignature(name, inputSignature, outputSignature, mode, deprecated, allowed, description, warning );
}
}

Expand Down
Expand Up @@ -140,18 +140,18 @@ public void registerProcedure( Class<?> proc ) throws KernelException
*/
public void registerProcedure( Class<?> proc, boolean overrideCurrentImplementation ) throws KernelException
{
registerProcedure( proc, overrideCurrentImplementation, false );
registerProcedure( proc, overrideCurrentImplementation, "" );
}

/**
* Register a new procedure defined with annotations on a java class.
* @param proc the procedure class
* @param overrideCurrentImplementation set to true if procedures within this class should override older procedures with the same name
* @param warn the warning the procedure should generate when called
* @param warning the warning the procedure should generate when called
*/
public void registerProcedure( Class<?> proc, boolean overrideCurrentImplementation, boolean warn ) throws KernelException
public void registerProcedure( Class<?> proc, boolean overrideCurrentImplementation, String warning ) throws KernelException
{
for ( CallableProcedure procedure : compiler.compileProcedure( proc, warn ) )
for ( CallableProcedure procedure : compiler.compileProcedure( proc, warning ) )
{
register( procedure, overrideCurrentImplementation );
}
Expand Down
Expand Up @@ -154,10 +154,10 @@ List<CallableUserAggregationFunction> compileAggregationFunction( Class<?> fcnDe

List<CallableProcedure> compileProcedure( Class<?> procDefinition ) throws KernelException
{
return compileProcedure( procDefinition, false );
return compileProcedure( procDefinition, "" );
}

List<CallableProcedure> compileProcedure( Class<?> procDefinition, boolean warn ) throws KernelException
List<CallableProcedure> compileProcedure( Class<?> procDefinition, String warning ) throws KernelException
{
try
{
Expand All @@ -175,7 +175,7 @@ List<CallableProcedure> compileProcedure( Class<?> procDefinition, boolean warn
ArrayList<CallableProcedure> out = new ArrayList<>( procedureMethods.size() );
for ( Method method : procedureMethods )
{
out.add( compileProcedure( procDefinition, constructor, method, warn ) );
out.add( compileProcedure( procDefinition, constructor, method, warning ) );
}
out.sort( Comparator.comparing( a -> a.signature().name().toString() ) );
return out;
Expand All @@ -191,7 +191,7 @@ List<CallableProcedure> compileProcedure( Class<?> procDefinition, boolean warn
}
}

private ReflectiveProcedure compileProcedure( Class<?> procDefinition, MethodHandle constructor, Method method, boolean warn )
private ReflectiveProcedure compileProcedure( Class<?> procDefinition, MethodHandle constructor, Method method, String warning )
throws ProcedureException, IllegalAccessException
{
String valueName = method.getAnnotation( Procedure.class ).value();
Expand Down Expand Up @@ -222,6 +222,8 @@ private ReflectiveProcedure compileProcedure( Class<?> procDefinition, MethodHan
Optional<String> deprecated = deprecated( method, procedure::deprecatedBy,
"Use of @Procedure(deprecatedBy) without @Deprecated in " + procName );

Optional<String> warn = (warning == null || warning.isEmpty()) ? Optional.empty() : Optional.of( warning );

ProcedureSignature signature =
new ProcedureSignature( procName, inputSignature, outputMapper.signature(),
mode, deprecated, config.rolesFor( procName.toString() ), description, warn );
Expand Down
Expand Up @@ -110,7 +110,7 @@ public void setup( Dependencies dependencies ) throws KernelException
ctx -> authManager.getUserManager( asEnterprise( ctx.get( SECURITY_CONTEXT ) ) ) );
if ( config.get( SecuritySettings.auth_providers ).size() > 1 )
{
procedures.registerProcedure( UserManagementProcedures.class, true, true );
procedures.registerProcedure( UserManagementProcedures.class, true, "%s only affect native users." );
}
else
{
Expand Down
Expand Up @@ -28,6 +28,7 @@
import org.neo4j.graphdb.Notification;
import org.neo4j.graphdb.Result;
import org.neo4j.kernel.api.KernelTransaction;
import org.neo4j.kernel.api.exceptions.Status;
import org.neo4j.kernel.impl.coreapi.InternalTransaction;
import org.neo4j.kernel.impl.factory.GraphDatabaseFacade;
import org.neo4j.server.security.enterprise.configuration.SecuritySettings;
Expand Down Expand Up @@ -108,27 +109,27 @@ public void shouldWarnWhenUsingNativeAndOtherProvider() throws Throwable
.beginTransaction( KernelTransaction.Type.explicit, StandardEnterpriseSecurityContext.AUTH_DISABLED );
Result result =
localGraph.execute( transaction, "EXPLAIN CALL dbms.security.listUsers", Collections.emptyMap() );
assertThat(
containsNotification( result, "Native user management procedures will not affect non-native users." ),
equalTo( true ) );
String description = String.format( "%s (%s)", Status.Procedure.ProcedureWarning.code().description(),
"dbms.security.listUsers only affect native users." );
assertThat( containsNotification( result, description ), equalTo( true ) );
transaction.success();
transaction.close();
}

@Test
public void shouldNotWarnWhenUsingNativeAndOtherProvider() throws Throwable
public void shouldNotWarnWhenOnlyUsingNativeProvider() throws Throwable
{
configuredSetup( stringMap( SecuritySettings.auth_provider.name(), "native" ) );
assertSuccess( adminSubject, "CALL dbms.security.listUsers", r -> assertKeyIsMap( r, "username", "roles", userList ) );

assertSuccess( adminSubject, "CALL dbms.security.listUsers",
r -> assertKeyIsMap( r, "username", "roles", userList ) );
GraphDatabaseFacade localGraph = neo.getLocalGraph();
InternalTransaction transaction = localGraph
.beginTransaction( KernelTransaction.Type.explicit, StandardEnterpriseSecurityContext.AUTH_DISABLED );
Result result =
localGraph.execute( transaction, "EXPLAIN CALL dbms.security.listUsers", Collections.emptyMap() );
assertThat(
containsNotification( result, "Native user management procedures will not affect non-native users." ),
equalTo( false ) );
String description = String.format( "%s (%s)", Status.Procedure.ProcedureWarning.code().description(),
"dbms.security.listUsers only affect native users." );
assertThat( containsNotification( result, description ), equalTo( false ) );
transaction.success();
transaction.close();
}
Expand Down

0 comments on commit 99b3643

Please sign in to comment.