Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 1502B200B13 for ; Wed, 15 Jun 2016 17:58:49 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id 13CF1160A4D; Wed, 15 Jun 2016 15:58:49 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id DE207160A57 for ; Wed, 15 Jun 2016 17:58:47 +0200 (CEST) Received: (qmail 25456 invoked by uid 500); 15 Jun 2016 15:40:42 -0000 Mailing-List: contact commits-help@brooklyn.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@brooklyn.apache.org Delivered-To: mailing list commits@brooklyn.apache.org Received: (qmail 25446 invoked by uid 99); 15 Jun 2016 15:40:41 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 15 Jun 2016 15:40:41 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id BE422E01E2; Wed, 15 Jun 2016 15:40:39 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit From: aledsage@apache.org To: commits@brooklyn.apache.org Date: Wed, 15 Jun 2016 15:40:40 -0000 Message-Id: <492e5e540ef347ee8c99d7e3043f09ea@git.apache.org> In-Reply-To: <62f1fb4e47a84f2d84c39169c394e850@git.apache.org> References: <62f1fb4e47a84f2d84c39169c394e850@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: =?utf-8?q?=5B2/3=5D_brooklyn-server_git_commit=3A_BROOKLYN-282=3A_?= =?utf-8?b?ZG9u4oCZdCBsb2cgcGFzc3dvcmRzICgjMSk=?= archived-at: Wed, 15 Jun 2016 15:58:49 -0000 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 Authored: Wed Jun 15 12:02:46 2016 +0100 Committer: Valentin Aitken 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 extends AbstractEffector { @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)parameters); } else { // we are dealing with a proxy here // this implementation invokes the method on the proxy @@ -158,6 +157,7 @@ public class MethodEffector extends AbstractEffector { // 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 invokeEffectorMethodLocal(Entity entity, Effector eff, Object args) { + protected T invokeEffectorMethodLocal(Entity entity, Effector eff, Map 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 invokeEffectorMethodSync(final Entity entity, final Effector eff, final Object args) throws ExecutionException { + public T invokeEffectorMethodSync(final Entity entity, final Effector eff, final Map 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 invokeMethodEffector(Entity entity, Effector eff, Object[] args) { + public static T invokeMethodEffector(Entity entity, Effector eff, Map 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 invokeEffectorMethodSync(final Entity entity, final Effector eff, final Object args) throws ExecutionException; + T invokeEffectorMethodSync(final Entity entity, final Effector eff, final Map args) throws ExecutionException; Task invokeEffector(final Entity entity, final Effector 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 invokeEffectorMethodSync(final Entity entity, final Effector eff, final Object args) throws ExecutionException { + public T invokeEffectorMethodSync(final Entity entity, final Effector eff, final Map 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: + *
    + *
  • excluded from the activities view + *
  • not logged + *
  • masked out in the UI + *
+ * Of those, only the first is unit-tested. + * + * To test manually... + * + * Configure logback to log everything at trace: + * 
+ * {@code
+ * 
+ *     
+ *     
+ *     
+ * 
+ * }
+ *
+ * + * Ensure that {@code TestEntityWithEffectors} (and its super-class {@TestEntity}) is on the + * classpath, and then run Brooklyn with the above log configuration file: + * 
+ * {@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
+ * }
+ *
+ * + * Deploy the blueprint below: + * 
+ * {@code
+ * services:
+ * - type: org.apache.brooklyn.core.mgmt.internal.TestEntityWithEffectors
+ *   brooklyn.children:
+ *   - type: org.apache.brooklyn.core.mgmt.internal.TestEntityWithEffectors
+ * }
+ *
+ * + * 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: + *
    + *
  • the parameter values were masked out while being entered + *
  • activity view of each TestEntityWithEffectors, that it does not show those parameter values + *
  • {@code grep -E "mypassword|12345678" *.log} + *
  • {@code pushd /path/to/persistedState && grep -r -E "mypassword|12345678" *} + *
+ */ @ImplementedBy(TestEntityWithEffectorsImpl.class) public interface TestEntityWithEffectors extends TestEntity { + + org.apache.brooklyn.api.effector.Effector RESET_PASSWORD = new MethodEffector(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); + } } }