brooklyn-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aleds...@apache.org
Subject [2/3] brooklyn-server git commit: BROOKLYN-282: don’t log passwords (#1)
Date Wed, 15 Jun 2016 15:40:40 GMT
BROOKLYN-282: don’t log passwords (#1)

Also refactors EffectorUtils.prepareArgsForEffector (deprecating old
method), and updates how MethodEffector calls it so we have the
parameter names.


Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo
Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/dfd6ef47
Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/dfd6ef47
Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/dfd6ef47

Branch: refs/heads/master
Commit: dfd6ef47ae8aafb71bddfeb504666790642ba81b
Parents: 3c0c8af
Author: Aled Sage <aled.sage@gmail.com>
Authored: Wed Jun 15 12:02:46 2016 +0100
Committer: Valentin Aitken <bostko@gmail.com>
Committed: Wed Jun 15 15:57:26 2016 +0300

----------------------------------------------------------------------
 .../brooklyn/core/effector/MethodEffector.java  |  4 +-
 .../internal/AbstractManagementContext.java     |  4 +-
 .../core/mgmt/internal/EffectorUtils.java       | 56 ++++++++++------
 .../internal/ManagementContextInternal.java     |  2 +-
 .../NonDeploymentManagementContext.java         |  2 +-
 .../mgmt/internal/TestEntityWithEffectors.java  | 68 +++++++++++++++++++-
 .../internal/TestEntityWithEffectorsImpl.java   | 24 +++++--
 7 files changed, 128 insertions(+), 32 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/dfd6ef47/core/src/main/java/org/apache/brooklyn/core/effector/MethodEffector.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/effector/MethodEffector.java b/core/src/main/java/org/apache/brooklyn/core/effector/MethodEffector.java
index ad53adb..0d5f909 100644
--- a/core/src/main/java/org/apache/brooklyn/core/effector/MethodEffector.java
+++ b/core/src/main/java/org/apache/brooklyn/core/effector/MethodEffector.java
@@ -144,9 +144,8 @@ public class MethodEffector<T> extends AbstractEffector<T>
{
 
     @SuppressWarnings({ "rawtypes", "unchecked" })
     public T call(Entity entity, Map parameters) {
-        Object[] parametersArray = EffectorUtils.prepareArgsForEffector(this, parameters);
         if (entity instanceof AbstractEntity) {
-            return EffectorUtils.invokeMethodEffector(entity, this, parametersArray);
+            return EffectorUtils.invokeMethodEffector(entity, this, (Map<String,?>)parameters);
         } else {
             // we are dealing with a proxy here
             // this implementation invokes the method on the proxy
@@ -158,6 +157,7 @@ public class MethodEffector<T> extends AbstractEffector<T>
{
             
             // TODO Should really find method with right signature, rather than just the
right args.
             // TODO prepareArgs can miss things out that have "default values"! Code below
will probably fail if that happens.
+            Object[] parametersArray = EffectorUtils.prepareArgsForEffector(this, parameters);
             Method[] methods = entity.getClass().getMethods();
             for (Method method : methods) {
                 if (method.getName().equals(getName())) {

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/dfd6ef47/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java
b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java
index 75af42c..da2f30c 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java
@@ -302,7 +302,7 @@ public abstract class AbstractManagementContext implements ManagementContextInte
         return runAtEntity(entity, eff, parameters);
     }
     
-    protected <T> T invokeEffectorMethodLocal(Entity entity, Effector<T> eff,
Object args) {
+    protected <T> T invokeEffectorMethodLocal(Entity entity, Effector<T> eff,
Map<String, ?> args) {
         assert isManagedLocally(entity) : "cannot invoke effector method at "+this+" because
it is not managed here";
         totalEffectorInvocationCount.incrementAndGet();
         Object[] transformedArgs = EffectorUtils.prepareArgsForEffector(eff, args);
@@ -315,7 +315,7 @@ public abstract class AbstractManagementContext implements ManagementContextInte
      * @throws ExecutionException 
      */
     @Override
-    public <T> T invokeEffectorMethodSync(final Entity entity, final Effector<T>
eff, final Object args) throws ExecutionException {
+    public <T> T invokeEffectorMethodSync(final Entity entity, final Effector<T>
eff, final Map<String, ?> args) throws ExecutionException {
         try {
             Task<?> current = Tasks.current();
             if (current == null || !entity.equals(BrooklynTaskTags.getContextEntity(current))
|| !isManagedLocally(entity)) {

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/dfd6ef47/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EffectorUtils.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EffectorUtils.java
b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EffectorUtils.java
index f340041..d2a8424 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EffectorUtils.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EffectorUtils.java
@@ -60,21 +60,40 @@ public class EffectorUtils {
 
     private static final Logger log = LoggerFactory.getLogger(EffectorUtils.class);
 
+    /**
+     * Prepares arguments for an effector, taking an array, which should contain the arguments
in 
+     * order, optionally omitting those which have defaults defined.
+     */
+    public static Object[] prepareArgsForEffector(Effector<?> eff, Object[] args) {
+        return prepareArgsForEffectorFromArray(eff, args);
+    }
+
+    /**
+     * Prepares arguments for an effector, taking a map, which should contain the arguments
by 
+     * name, optionally omitting those which have defaults defined; performs type coercion
on
+     * the values.
+     */
+    public static Object[] prepareArgsForEffector(Effector<?> eff, Map<?,?> args)
{
+        return prepareArgsForEffectorFromMap(eff, (Map<?,?>) args);
+    }
+
     /** prepares arguments for an effector either accepting:
-     *  an array, which should contain the arguments in order, optionally omitting those
which have defaults defined;
-     *  or a map, which should contain the arguments by name, again optionally omitting those
which have defaults defined,
-     *  and in this case also performing type coercion.
+     *  an array (see {@link #prepareArgsForEffector(Effector, Object[])};
+     *  or a map (see {@link #prepareArgsForEffector(Effector, Map)}.
+     *  
+     * @deprecated since 0.10.0
      */
+    @Deprecated
     public static Object[] prepareArgsForEffector(Effector<?> eff, Object args) {
         if (args != null && args.getClass().isArray()) {
             return prepareArgsForEffectorFromArray(eff, (Object[]) args);
         }
         if (args instanceof Map) {
-            return prepareArgsForEffectorFromMap(eff, (Map) args);
+            return prepareArgsForEffectorFromMap(eff, (Map<?,?>) args);
         }
-        log.warn("Deprecated effector invocation style for call to "+eff+", expecting a map
or an array, got: "+sanitizeArgs(args));
+        log.warn("Deprecated effector invocation style for call to "+eff+", expecting a map
or an array, got: "+(args == null ? null : args.getClass().getName()));
         if (log.isDebugEnabled()) {
-            log.debug("Deprecated effector invocation style for call to "+eff+", expecting
a map or an array, got: "+sanitizeArgs(args),
+            log.debug("Deprecated effector invocation style for call to "+eff+", expecting
a map or an array, got: "+(args == null ? null : args.getClass().getName()),
                 new Throwable("Trace for deprecated effector invocation style"));
         }
         return oldPrepareArgsForEffector(eff, args);
@@ -115,16 +134,16 @@ public class EffectorUtils {
                 //finally, default values are used to make up for missing parameters
                 newArgs.put(it.getName(), ((BasicParameterType)it).getDefaultValue());
             } else {
-                throw new IllegalArgumentException("Invalid arguments (count mismatch) for
effector "+eff+": "+args);
+                throw new IllegalArgumentException("Invalid arguments (count mismatch) for
effector "+eff+": "+args.length+" args");
             }
 
             newArgsNeeded--;
         }
         if (newArgsNeeded > 0) {
-            throw new IllegalArgumentException("Invalid arguments (missing "+newArgsNeeded+")
for effector "+eff+": "+args);
+            throw new IllegalArgumentException("Invalid arguments (missing "+newArgsNeeded+")
for effector "+eff+": "+args.length+" args");
         }
         if (!l.isEmpty()) {
-            throw new IllegalArgumentException("Invalid arguments ("+l.size()+" extra) for
effector "+eff+": "+args);
+            throw new IllegalArgumentException("Invalid arguments ("+l.size()+" extra) for
effector "+eff+": "+args.length+" args");
         }
         return newArgs;
     }
@@ -219,19 +238,19 @@ public class EffectorUtils {
                 //finally, default values are used to make up for missing parameters
                 newArgs.add(((BasicParameterType)it).getDefaultValue());
             } else {
-                throw new IllegalArgumentException("Invalid arguments (count mismatch) for
effector "+eff+": "+sanitizeArgs(args));
+                throw new IllegalArgumentException("Invalid arguments (count mismatch) for
effector "+eff+": "+argsArray.length+" args");
             }
 
             newArgsNeeded--;
         }
         if (newArgsNeeded > 0) {
-            throw new IllegalArgumentException("Invalid arguments (missing "+newArgsNeeded+")
for effector "+eff+": "+sanitizeArgs(args));
+            throw new IllegalArgumentException("Invalid arguments (missing "+newArgsNeeded+")
for effector "+eff+": "+argsArray.length+" args");
         }
         if (!l.isEmpty()) {
-            throw new IllegalArgumentException("Invalid arguments ("+l.size()+" extra) for
effector "+eff+": "+sanitizeArgs(args));
+            throw new IllegalArgumentException("Invalid arguments ("+l.size()+" extra) for
effector "+eff+": "+argsArray.length+" args");
         }
         if (truth(m) && !mapUsed) {
-            throw new IllegalArgumentException("Invalid arguments ("+m.size()+" extra named)
for effector "+eff+": "+sanitizeArgs(args));
+            throw new IllegalArgumentException("Invalid arguments ("+m.size()+" extra named)
for effector "+eff+": "+argsArray.length+" args");
         }
         return newArgs.toArray(new Object[newArgs.size()]);
     }
@@ -239,19 +258,20 @@ public class EffectorUtils {
     /**
      * Invokes a method effector so that its progress is tracked. For internal use only,
when we know the effector is backed by a method which is local.
      */
-    public static <T> T invokeMethodEffector(Entity entity, Effector<T> eff,
Object[] args) {
+    public static <T> T invokeMethodEffector(Entity entity, Effector<T> eff,
Map<String,?> args) {
+        Object[] parametersArray = EffectorUtils.prepareArgsForEffector(eff, args);
         String name = eff.getName();
 
         try {
             if (log.isDebugEnabled()) log.debug("Invoking effector {} on {}", new Object[]
{name, entity});
-            if (log.isTraceEnabled()) log.trace("Invoking effector {} on {} with args {}",
new Object[] {name, entity, args});
+            if (log.isTraceEnabled()) log.trace("Invoking effector {} on {} with args {}",
new Object[] {name, entity, Sanitizer.sanitize(args)});
             EntityManagementSupport mgmtSupport = ((EntityInternal)entity).getManagementSupport();
             if (!mgmtSupport.isDeployed()) {
                 mgmtSupport.attemptLegacyAutodeployment(name);
             }
             ManagementContextInternal mgmtContext = (ManagementContextInternal) ((EntityInternal)
entity).getManagementContext();
 
-            mgmtSupport.getEntityChangeListener().onEffectorStarting(eff, args);
+            mgmtSupport.getEntityChangeListener().onEffectorStarting(eff, parametersArray);
             try {
                 return mgmtContext.invokeEffectorMethodSync(entity, eff, args);
             } finally {
@@ -392,8 +412,4 @@ public class EffectorUtils {
                 .put("tags", tags)
                 .build();
     }
-
-    private static Object sanitizeArgs(Object args) {
-        return args instanceof Map ? Sanitizer.sanitize((Map)args) : args;
-    }
 }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/dfd6ef47/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/ManagementContextInternal.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/ManagementContextInternal.java
b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/ManagementContextInternal.java
index e76f2fb..dd3c9d5 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/ManagementContextInternal.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/ManagementContextInternal.java
@@ -69,7 +69,7 @@ public interface ManagementContextInternal extends ManagementContext {
     
     long getTotalEffectorInvocations();
 
-    <T> T invokeEffectorMethodSync(final Entity entity, final Effector<T> eff,
final Object args) throws ExecutionException;
+    <T> T invokeEffectorMethodSync(final Entity entity, final Effector<T> eff,
final Map<String,?> args) throws ExecutionException;
     
     <T> Task<T> invokeEffector(final Entity entity, final Effector<T> eff,
@SuppressWarnings("rawtypes") final Map parameters);
 

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/dfd6ef47/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/NonDeploymentManagementContext.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/NonDeploymentManagementContext.java
b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/NonDeploymentManagementContext.java
index 68172bb..cfc1d98 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/NonDeploymentManagementContext.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/NonDeploymentManagementContext.java
@@ -349,7 +349,7 @@ public class NonDeploymentManagementContext implements ManagementContextInternal
     }
     
     @Override
-    public <T> T invokeEffectorMethodSync(final Entity entity, final Effector<T>
eff, final Object args) throws ExecutionException {
+    public <T> T invokeEffectorMethodSync(final Entity entity, final Effector<T>
eff, final Map<String,?> args) throws ExecutionException {
         throw new IllegalStateException("Non-deployment context "+this+" is not valid for
this operation: cannot invoke effector "+eff+" on entity "+entity);
     }
     

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/dfd6ef47/core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectors.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectors.java
b/core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectors.java
index f986674..4bb137d 100644
--- a/core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectors.java
+++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectors.java
@@ -21,15 +21,79 @@ package org.apache.brooklyn.core.mgmt.internal;
 import org.apache.brooklyn.api.entity.ImplementedBy;
 import org.apache.brooklyn.core.annotation.Effector;
 import org.apache.brooklyn.core.annotation.EffectorParam;
+import org.apache.brooklyn.core.effector.MethodEffector;
 import org.apache.brooklyn.core.test.entity.TestEntity;
 
+/**
+ * Entity for testing that secret effector parameters are:
+ * <ul>
+ *   <li>excluded from the activities view
+ *   <li>not logged
+ *   <li>masked out in the UI
+ * </ul>
+ * Of those, only the first is unit-tested.
+ * 
+ * To test manually...
+ * 
+ * Configure logback to log everything at trace:
+ * <pre>
+ * {@code
+ * <configuration>
+ *     <include resource="logback-main.xml"/>
+ *     <logger name="org.apache.brooklyn" level="TRACE"/>
+ *     <logger name="brooklyn" level="TRACE"/>
+ * </configuration>
+ * }
+ * </pre>
+ *
+ * Ensure that {@code TestEntityWithEffectors} (and its super-class {@TestEntity}) is on
the
+ * classpath, and then run Brooklyn with the above log configuration file:
+ * <pre>
+ * {@code
+ * cp /path/to/test-entities.jar ./lib/dropins/
+ * export JAVA_OPTS="-Xms256m -Xmx1g -XX:MaxPermSize=256m -Dlogback.configurationFile=/path/to/logback-trace.xml"
+ * ./bin/brooklyn launch --persist auto --persistenceDir /path/to/persistedState
+ * }
+ * </pre>
+ *
+ * Deploy the blueprint below:
+ * <pre>
+ * {@code
+ * services:
+ * - type: org.apache.brooklyn.core.mgmt.internal.TestEntityWithEffectors
+ *   brooklyn.children:
+ *   - type: org.apache.brooklyn.core.mgmt.internal.TestEntityWithEffectors
+ * }
+ * </pre>
+ * 
+ * Invoke the effector (e.g. {@code resetPasswordOnChildren("mypassword", 12345678)} on the

+ * top-level {@code TestEntityWithEffectors}, and the effector 
+ * {@code resetPasswordThrowsException("mypassword", 12345678)}).
+ * 
+ * Then manually check:
+ * <ul>
+ *   <li>the parameter values were masked out while being entered
+ *   <li>activity view of each TestEntityWithEffectors, that it does not show those
parameter values
+ *   <li>{@code grep -E "mypassword|12345678" *.log}
+ *   <li>{@code pushd /path/to/persistedState && grep -r -E "mypassword|12345678"
*}
+ * </ul>
+ */
 @ImplementedBy(TestEntityWithEffectorsImpl.class)
 public interface TestEntityWithEffectors extends TestEntity {
+    
+    org.apache.brooklyn.api.effector.Effector<Void> RESET_PASSWORD = new MethodEffector<Void>(TestEntityWithEffectors.class,
"resetPassword");
+    
     @Effector(description = "Reset password")
-    Void resetPassword(@EffectorParam(name = "newPassword") String param1, @EffectorParam(name
= "secretPin") Integer secretPin);
+    void resetPassword(@EffectorParam(name = "newPassword") String newPassword, @EffectorParam(name
= "secretPin") Integer secretPin);
+
+    @Effector(description = "Reset password throws exception")
+    void resetPasswordThrowsException(@EffectorParam(name = "newPassword") String newPassword,
@EffectorParam(name = "secretPin") Integer secretPin);
 
     @Effector(description = "Reset User and password effector")
-    Void invokeUserAndPassword(@EffectorParam(name = "user") String user,
+    void invokeUserAndPassword(@EffectorParam(name = "user") String user,
                                @EffectorParam(name = "newPassword", defaultValue = "Test")
String newPassword,
                                @EffectorParam(name = "paramDefault", defaultValue = "DefaultValue")
String paramDefault);
+    
+    @Effector(description = "Reset password on children")
+    void resetPasswordOnChildren(@EffectorParam(name = "newPassword") String newPassword,
@EffectorParam(name = "secretPin") Integer secretPin) throws Exception;
 }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/dfd6ef47/core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectorsImpl.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectorsImpl.java
b/core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectorsImpl.java
index 8566d59..dd270ad 100644
--- a/core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectorsImpl.java
+++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectorsImpl.java
@@ -18,25 +18,41 @@
  */
 package org.apache.brooklyn.core.mgmt.internal;
 
+import org.apache.brooklyn.core.entity.Entities;
 import org.apache.brooklyn.core.test.entity.TestEntityImpl;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+
 public class TestEntityWithEffectorsImpl extends TestEntityImpl implements TestEntityWithEffectors
{
     private static final Logger log = LoggerFactory.getLogger(TestEntityWithEffectorsImpl.class);
 
     @Override
-    public Void resetPassword(String newPassword, Integer secretPin) {
+    public void resetPassword(String newPassword, Integer secretPin) {
         log.info("Invoked effector from resetPassword with params {} {}", new Object[] {newPassword,
secretPin});
         assert newPassword != null;
-        return null;
     }
 
     @Override
-    public Void invokeUserAndPassword(String user,String newPassword, String paramDefault)
{
+    public void resetPasswordThrowsException(String newPassword, Integer secretPin) {
+        log.info("Invoked effector from resetPasswordThrowsException with params {} {}",
new Object[] {newPassword, secretPin});
+        throw new RuntimeException("Simulate failure in resetPasswordThrowsException");
+    }
+    
+    @Override
+    public void invokeUserAndPassword(String user,String newPassword, String paramDefault)
{
         log.info("Invoked effector from invokeUserAndPassword with params {} {} {}", new
Object[] {user, newPassword, paramDefault});
         assert user != null;
         assert newPassword != null;
-        return null;
+    }
+
+    @Override
+    public void resetPasswordOnChildren(String newPassword, Integer secretPin) throws Exception
{
+        for (TestEntityWithEffectors child : Iterables.filter(getChildren(), TestEntityWithEffectors.class))
{
+            Entities.invokeEffector(this, child, RESET_PASSWORD, ImmutableMap.of("newPassword",
newPassword, "secretPin", secretPin)).get();
+            child.resetPassword(newPassword, secretPin);
+        }
     }
 }


Mime
View raw message