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.s...@gmail.com> Authored: Wed Jun 15 12:02:46 2016 +0100 Committer: Valentin Aitken <bos...@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); + } } }
