geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kl...@apache.org
Subject [38/50] [abbrv] incubator-geode git commit: GEODE-17: clean up error messages
Date Fri, 20 May 2016 17:11:03 GMT
GEODE-17: clean up error messages

* clean up authentication/authorization error messages
* Catch Authorization exception later in the command chain to avoid unnecesary parsing of
command result
* Add ExceptionHandler in controller to set the http header correctly
* Catch Authorization exception in gfsh execution for better error report


Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/14a548ff
Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/14a548ff
Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/14a548ff

Branch: refs/heads/feature/GEODE-835
Commit: 14a548ff22d7055f1da75e5432412472113e5e9d
Parents: 21c0e24
Author: Jinmei Liao <jiliao@pivotal.io>
Authored: Tue May 17 14:48:30 2016 -0700
Committer: Jinmei Liao <jiliao@pivotal.io>
Committed: Thu May 19 10:40:57 2016 -0700

----------------------------------------------------------------------
 .../internal/security/GeodeSecurityUtil.java    |  55 ++---
 .../internal/beans/MemberMBeanBridge.java       |  18 +-
 .../internal/cli/remote/CommandProcessor.java   |   9 +-
 .../internal/cli/result/ResultBuilder.java      |   3 +-
 .../cli/shell/GfshExecutionStrategy.java        | 230 ++++++++++---------
 .../security/ResourceOperationContext.java      |   2 +-
 .../controllers/AbstractCommandsController.java |  88 ++-----
 .../web/shell/AbstractHttpOperationInvoker.java |  16 +-
 .../security/GfshCommandsSecurityTest.java      |  11 +-
 .../security/MemberMBeanSecurityJUnitTest.java  |   2 +-
 .../WanCommandsControllerJUnitTest.java         |   7 +-
 11 files changed, 199 insertions(+), 242 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/14a548ff/geode-core/src/main/java/com/gemstone/gemfire/internal/security/GeodeSecurityUtil.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/security/GeodeSecurityUtil.java
b/geode-core/src/main/java/com/gemstone/gemfire/internal/security/GeodeSecurityUtil.java
index 236b00b..322c59e 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/internal/security/GeodeSecurityUtil.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/security/GeodeSecurityUtil.java
@@ -33,6 +33,7 @@ import com.gemstone.gemfire.management.internal.security.ResourceOperation;
 import com.gemstone.gemfire.management.internal.security.ResourceOperationContext;
 import com.gemstone.gemfire.security.AuthenticationFailedException;
 import com.gemstone.gemfire.security.GemFireSecurityException;
+import com.gemstone.gemfire.security.NotAuthorizedException;
 
 import org.apache.commons.lang.StringUtils;
 import org.apache.logging.log4j.Logger;
@@ -53,31 +54,6 @@ public class GeodeSecurityUtil {
   private static Logger logger = LogService.getLogger();
 
   /**
-   *
-   * @param username
-   * @param password
-   * @return null if security is not enabled, otherwise return a shiro subject
-   */
-  public static Subject login(String username, String password){
-    if(!isSecured())
-      return null;
-
-    Subject currentUser = SecurityUtils.getSubject();
-
-    UsernamePasswordToken token =
-        new UsernamePasswordToken(username, password);
-    try {
-      logger.info("Logging in "+username+"/"+password);
-      currentUser.login(token);
-    } catch (ShiroException e) {
-      logger.info(e.getMessage(), e);
-      throw new AuthenticationFailedException(e.getMessage(), e);
-    }
-
-    return currentUser;
-  }
-
-  /**
    * It first looks the shiro subject in AccessControlContext since JMX will use multiple
threads to process operations from the same client.
    * then it looks into Shiro's thead context.
    *
@@ -114,6 +90,31 @@ public class GeodeSecurityUtil {
     return currentUser;
   }
 
+  /**
+   *
+   * @param username
+   * @param password
+   * @return null if security is not enabled, otherwise return a shiro subject
+   */
+  public static Subject login(String username, String password){
+    if(!isSecured())
+      return null;
+
+    Subject currentUser = SecurityUtils.getSubject();
+
+    UsernamePasswordToken token =
+      new UsernamePasswordToken(username, password);
+    try {
+      logger.info("Logging in "+username+"/"+password);
+      currentUser.login(token);
+    } catch (ShiroException e) {
+      logger.info(e.getMessage(), e);
+      throw new AuthenticationFailedException("Authentication error. Please check your username/password.",
e);
+    }
+
+    return currentUser;
+  }
+
   public static void logout(){
     Subject currentUser = getSubject();
     if(currentUser==null)
@@ -125,7 +126,7 @@ public class GeodeSecurityUtil {
     }
     catch(ShiroException e){
       logger.info(e.getMessage(), e);
-      throw new AuthenticationFailedException(e.getMessage(), e);
+      throw new GemFireSecurityException(e.getMessage(), e);
     }
     // clean out Shiro's thread local content
     ThreadContext.remove();
@@ -205,7 +206,7 @@ public class GeodeSecurityUtil {
     }
     catch(ShiroException e){
       logger.info(currentUser.getPrincipal() + " not authorized for " + context);
-      throw new GemFireSecurityException(e.getMessage(), e);
+      throw new NotAuthorizedException(e.getMessage(), e);
     }
   }
 

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/14a548ff/geode-core/src/main/java/com/gemstone/gemfire/management/internal/beans/MemberMBeanBridge.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/beans/MemberMBeanBridge.java
b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/beans/MemberMBeanBridge.java
index 67ad60d..98258e8 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/beans/MemberMBeanBridge.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/beans/MemberMBeanBridge.java
@@ -123,10 +123,9 @@ import com.gemstone.gemfire.management.internal.cli.CommandResponseBuilder;
 import com.gemstone.gemfire.management.internal.cli.remote.CommandExecutionContext;
 import com.gemstone.gemfire.management.internal.cli.remote.MemberCommandService;
 import com.gemstone.gemfire.management.internal.cli.result.CommandResult;
-import com.gemstone.gemfire.management.internal.cli.result.ErrorResultData;
 import com.gemstone.gemfire.management.internal.cli.result.ResultBuilder;
 import com.gemstone.gemfire.management.internal.cli.shell.Gfsh;
-import com.gemstone.gemfire.security.GemFireSecurityException;
+
 import org.apache.logging.log4j.Logger;
 
 /**
@@ -1738,17 +1737,8 @@ public class MemberMBeanBridge {
     }
 
     if (isGfshRequest) {
-      String responseJson = CommandResponseBuilder.createCommandResponseJson(getMember(),
(CommandResult) result);
-  //    System.out.println("responseJson :: "+responseJson);
-      return responseJson;
+      return CommandResponseBuilder.createCommandResponseJson(getMember(), (CommandResult)
result);
     } else {
-      // throw GemFireSecurityException is the returned error code is 415
-      if(((CommandResult) result).getResultData() instanceof ErrorResultData){
-        ErrorResultData resultData = (ErrorResultData) ((CommandResult)result).getResultData();
-        if(resultData.getErrorCode()==ResultBuilder.ERRORCODE_UNAUTHORIZED){
-          throw new GemFireSecurityException(resultData.getGfJsonObject().toString());
-        }
-      }
       return ResultBuilder.resultAsString(result);
     }
   }
@@ -1758,14 +1748,12 @@ public class MemberMBeanBridge {
     if (env != null) {
       appName = env.get(Gfsh.ENV_APP_NAME);
     }
-//    System.out.println("appName :: "+appName);
     
     return Gfsh.GFSH_APP_NAME.equals(appName);
   }
   
   public long getTotalDiskUsage() {
-    long diskSpaceUsage = regionMonitor.getDiskSpace();
-    return diskSpaceUsage;
+    return regionMonitor.getDiskSpace();
   }
 
 

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/14a548ff/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/remote/CommandProcessor.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/remote/CommandProcessor.java
b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/remote/CommandProcessor.java
index 7edc3e4..69bc64e 100755
--- a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/remote/CommandProcessor.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/remote/CommandProcessor.java
@@ -21,6 +21,7 @@ import java.lang.reflect.Method;
 import java.util.Map;
 import java.util.Properties;
 
+import com.gemstone.gemfire.internal.security.GeodeSecurityUtil;
 import com.gemstone.gemfire.management.cli.CommandProcessingException;
 import com.gemstone.gemfire.management.cli.CommandStatement;
 import com.gemstone.gemfire.management.cli.Result;
@@ -30,8 +31,7 @@ import com.gemstone.gemfire.management.internal.cli.LogWrapper;
 import com.gemstone.gemfire.management.internal.cli.result.ResultBuilder;
 import com.gemstone.gemfire.management.internal.cli.util.CommentSkipHelper;
 import com.gemstone.gemfire.management.internal.security.ResourceOperation;
-import com.gemstone.gemfire.security.GemFireSecurityException;
-import com.gemstone.gemfire.internal.security.GeodeSecurityUtil;
+import com.gemstone.gemfire.security.NotAuthorizedException;
 
 import org.springframework.shell.core.Parser;
 import org.springframework.shell.event.ParseResult;
@@ -126,12 +126,13 @@ public class CommandProcessor {
           logWrapper.info("Could not parse \""+cmdStmt.getCommandString()+"\".", e);
         }
         return ResultBuilder.createParsingErrorResult(e.getMessage());
-      } catch (GemFireSecurityException e) {
+      } catch (NotAuthorizedException e) {
         setLastExecutionStatus(1);
         if (logWrapper.infoEnabled()) {
           logWrapper.info("Could not execute \""+cmdStmt.getCommandString()+"\".", e);
         }
-        return ResultBuilder.createGemFireUnAuthorizedErrorResult("Unauthorized while processing
command <" +cmdStmt.getCommandString()+"> Reason : " + e.getMessage());
+        // for NotAuthorizedException, will catch this later in the code
+        throw e;
       }catch (RuntimeException e) {
         setLastExecutionStatus(1);
         if (logWrapper.infoEnabled()) {

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/14a548ff/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/result/ResultBuilder.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/result/ResultBuilder.java
b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/result/ResultBuilder.java
index 6b435d3..55f7a89 100755
--- a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/result/ResultBuilder.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/result/ResultBuilder.java
@@ -116,8 +116,7 @@ public class ResultBuilder {
   }
 
   public static Result createGemFireUnAuthorizedErrorResult(String message) {
-    return createErrorResult(ERRORCODE_UNAUTHORIZED,
-        "Could not process command due to GemFire error. " + message);
+    return createErrorResult(ERRORCODE_UNAUTHORIZED, message);
   }
 
   public static Result createUserErrorResult(String message) {

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/14a548ff/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshExecutionStrategy.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshExecutionStrategy.java
b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshExecutionStrategy.java
index c5ebe9a..0cfae9c 100755
--- a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshExecutionStrategy.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshExecutionStrategy.java
@@ -16,17 +16,11 @@
  */
 package com.gemstone.gemfire.management.internal.cli.shell;
 
-import static com.gemstone.gemfire.management.internal.cli.multistep.CLIMultiStepHelper.execCLISteps;
+import static com.gemstone.gemfire.management.internal.cli.multistep.CLIMultiStepHelper.*;
 
 import java.lang.reflect.Method;
 import java.util.Map;
 
-import org.springframework.shell.core.ExecutionStrategy;
-import org.springframework.shell.core.Shell;
-import org.springframework.shell.event.ParseResult;
-import org.springframework.util.Assert;
-import org.springframework.util.ReflectionUtils;
-
 import com.gemstone.gemfire.internal.ClassPathLoader;
 import com.gemstone.gemfire.management.cli.CliMetaData;
 import com.gemstone.gemfire.management.cli.CommandProcessingException;
@@ -42,6 +36,13 @@ import com.gemstone.gemfire.management.internal.cli.i18n.CliStrings;
 import com.gemstone.gemfire.management.internal.cli.multistep.MultiStepCommand;
 import com.gemstone.gemfire.management.internal.cli.result.FileResult;
 import com.gemstone.gemfire.management.internal.cli.result.ResultBuilder;
+import com.gemstone.gemfire.security.NotAuthorizedException;
+
+import org.springframework.shell.core.ExecutionStrategy;
+import org.springframework.shell.core.Shell;
+import org.springframework.shell.event.ParseResult;
+import org.springframework.util.Assert;
+import org.springframework.util.ReflectionUtils;
 
 /**
  * Defines the {@link ExecutionStrategy} for commands that are executed in
@@ -81,43 +82,40 @@ public class GfshExecutionStrategy implements ExecutionStrategy {
     Object result = null;
     Method method = parseResult.getMethod();
     try {
-      
-    //Check if it's a multi-step command
+      //Check if it's a multi-step command
       Method reflectmethod = parseResult.getMethod();
-      MultiStepCommand cmd = reflectmethod.getAnnotation(MultiStepCommand.class);      
-      if(cmd!=null){
-        return execCLISteps(logWrapper, shell,parseResult);
+      MultiStepCommand cmd = reflectmethod.getAnnotation(MultiStepCommand.class);
+      if (cmd != null) {
+        return execCLISteps(logWrapper, shell, parseResult);
       }
 
-//      See #46072 
-//      String commandName = getCommandName(parseResult);
-//      if (commandName != null) {
-//        shell.flashMessage("Executing " + getCommandName(parseResult) + " ... ");
-//      }
-      //Check if it's a remote command
-      if (!isShellOnly(method)) {
-        if (GfshParseResult.class.isInstance(parseResult)) {
-          result = executeOnRemote((GfshParseResult)parseResult);
-        } else {//Remote command means implemented for Gfsh and ParseResult should be GfshParseResult.
-          //TODO - Abhishek: should this message be more specific?
-          throw new IllegalStateException("Configuration error!");
-        }
-      } else {
+      //check if it's a shell only command
+      if(isShellOnly(method)){
         Assert.notNull(parseResult, "Parse result required");
         synchronized (mutex) {
-          //TODO: Remove Assert
           Assert.isTrue(isReadyForCommands(), "ProcessManagerHostedExecutionStrategy not
yet ready for commands");
-          result = ReflectionUtils.invokeMethod(parseResult.getMethod(), parseResult.getInstance(),
parseResult.getArguments());
+          return ReflectionUtils.invokeMethod(parseResult.getMethod(), parseResult.getInstance(),
parseResult.getArguments());
         }
       }
-//    See #46072
-//      shell.flashMessage("");
-    } catch (JMXInvocationException e) {
+
+      //check if it's a GfshParseResult
+      if(!GfshParseResult.class.isInstance(parseResult)){
+        throw new IllegalStateException("Configuration error!");
+      }
+
+      result = executeOnRemote((GfshParseResult) parseResult);
+    }
+    catch(NotAuthorizedException e) {
+      result = ResultBuilder.createGemFireUnAuthorizedErrorResult("Unauthorized. Reason :
" + e.getMessage());
+    }
+    catch (JMXInvocationException e) {
       Gfsh.getCurrentInstance().logWarning(e.getMessage(), e);
-    } catch (IllegalStateException e) {
+    }
+    catch (IllegalStateException e) {
       // Shouldn't occur - we are always using GfsParseResult
       Gfsh.getCurrentInstance().logWarning(e.getMessage(), e);
-    } catch (CommandProcessingException e) {
+    }
+    catch (CommandProcessingException e) {
       Gfsh.getCurrentInstance().logWarning(e.getMessage(), null);
       Object errorData = e.getErrorData();
       if (errorData != null && errorData instanceof Throwable) {
@@ -125,16 +123,17 @@ public class GfshExecutionStrategy implements ExecutionStrategy {
       } else {
         logWrapper.warning(e.getMessage());
       }
-    } catch (RuntimeException e) {
+    }
+    catch (RuntimeException e) {
       Gfsh.getCurrentInstance().logWarning("Exception occurred. " + e.getMessage(), e);
       // Log other runtime exception in gfsh log
       logWrapper.warning("Error occurred while executing command : "+((GfshParseResult)parseResult).getUserInput(),
e);
-    } catch (Exception e) {
+    }
+    catch (Exception e) {
       Gfsh.getCurrentInstance().logWarning("Unexpected exception occurred. " + e.getMessage(),
e);
       // Log other exceptions in gfsh log
       logWrapper.warning("Unexpected error occurred while executing command : "+((GfshParseResult)parseResult).getUserInput(),
e);
     }
-    
     return result;
   }
   
@@ -204,90 +203,97 @@ public class GfshExecutionStrategy implements ExecutionStrategy {
   private Result executeOnRemote(GfshParseResult parseResult) {
     Result   commandResult = null;
     Object   response      = null;
-    if (shell.isConnectedAndReady()) {
-      byte[][]             fileData    = null;
-      CliAroundInterceptor interceptor = null;
-      
-      String interceptorClass = getInterceptor(parseResult.getMethod());
-      
-      //1. Pre Remote Execution
-      if (!CliMetaData.ANNOTATION_NULL_VALUE.equals(interceptorClass)) {
-        try {
-          interceptor = (CliAroundInterceptor) ClassPathLoader.getLatest().forName(interceptorClass).newInstance();
-        } catch (InstantiationException e) {
-          shell.logWarning("Configuration error", e);
-        } catch (IllegalAccessException e) {
-          shell.logWarning("Configuration error", e);
-        } catch (ClassNotFoundException e) {
-          shell.logWarning("Configuration error", e);
-        }
-        if (interceptor != null) {
-          Result preExecResult = interceptor.preExecution(parseResult);
-          if (Status.ERROR.equals(preExecResult.getStatus())) {
-            return preExecResult;
-          } else if (preExecResult instanceof FileResult) {            
-            FileResult fileResult = (FileResult) preExecResult;
-            fileData = fileResult.toBytes();
-          }
-        } else {
-          return ResultBuilder.createBadConfigurationErrorResult("Interceptor Configuration
Error");
+
+    if(!shell.isConnectedAndReady()){
+      shell.logWarning("Can't execute a remote command without connection. Use 'connect'
first to connect.", null);
+      logWrapper.info("Can't execute a remote command \""+parseResult.getUserInput()+"\"
without connection. Use 'connect' first to connect to GemFire.");
+      return null;
+    }
+
+    byte[][]             fileData    = null;
+    CliAroundInterceptor interceptor = null;
+
+    String interceptorClass = getInterceptor(parseResult.getMethod());
+
+    //1. Pre Remote Execution
+    if (!CliMetaData.ANNOTATION_NULL_VALUE.equals(interceptorClass)) {
+      try {
+        interceptor = (CliAroundInterceptor) ClassPathLoader.getLatest().forName(interceptorClass).newInstance();
+      } catch (InstantiationException e) {
+        shell.logWarning("Configuration error", e);
+      } catch (IllegalAccessException e) {
+        shell.logWarning("Configuration error", e);
+      } catch (ClassNotFoundException e) {
+        shell.logWarning("Configuration error", e);
+      }
+      if (interceptor != null) {
+        Result preExecResult = interceptor.preExecution(parseResult);
+        if (Status.ERROR.equals(preExecResult.getStatus())) {
+          return preExecResult;
+        } else if (preExecResult instanceof FileResult) {
+          FileResult fileResult = (FileResult) preExecResult;
+          fileData = fileResult.toBytes();
         }
+      } else {
+        return ResultBuilder.createBadConfigurationErrorResult("Interceptor Configuration
Error");
       }
+    }
 
-      //2. Remote Execution
-      final Map<String, String> env = shell.getEnv();
+    //2. Remote Execution
+    final Map<String, String> env = shell.getEnv();
+    try {
       response = shell.getOperationInvoker().processCommand(new CommandRequest(parseResult,
env, fileData));
+    } catch(NotAuthorizedException e) {
+      return ResultBuilder.createGemFireUnAuthorizedErrorResult("Unauthorized. Reason : "
+ e.getMessage());
+    }
+    finally {
       env.clear();
-      
-      if (response == null) {
-        shell.logWarning("Response was null for: \""+parseResult.getUserInput()+"\". (gfsh.isConnected="+shell.isConnectedAndReady()+")",
null);
-        commandResult = 
-            ResultBuilder.createBadResponseErrorResult(" Error occurred while " + 
-                "executing \""+parseResult.getUserInput()+"\" on manager. " +
-                		"Please check manager logs for error.");
-      } else {
-        if (logWrapper.fineEnabled()) {
-          logWrapper.fine("Received response :: "+response);
-        }
-        CommandResponse commandResponse = CommandResponseBuilder.prepareCommandResponseFromJson((String)
response);
-        
-        if (commandResponse.isFailedToPersist()) {
-          shell.printAsSevere(CliStrings.SHARED_CONFIGURATION_FAILED_TO_PERSIST_COMMAND_CHANGES);
-          logWrapper.severe(CliStrings.SHARED_CONFIGURATION_FAILED_TO_PERSIST_COMMAND_CHANGES);
-        }
-        
-        String debugInfo = commandResponse.getDebugInfo();
-        if (debugInfo != null && !debugInfo.trim().isEmpty()) {
-          //TODO - Abhishek When debug is ON, log response in gfsh logs
-          //TODO - Abhishek handle \n better. Is it coming from GemFire formatter
-          debugInfo = debugInfo.replaceAll("\n\n\n", "\n");
-          debugInfo = debugInfo.replaceAll("\n\n", "\n");
-          debugInfo = debugInfo.replaceAll("\n", "\n[From Manager : "+commandResponse.getSender()+"]");
-          debugInfo = "[From Manager : "+commandResponse.getSender()+"]" + debugInfo;
-          LogWrapper.getInstance().info(debugInfo);
-        }
-        commandResult = ResultBuilder.fromJson((String) response);
-        
-        
-        //3. Post Remote Execution
-        if (interceptor != null) {
-          Result postExecResult = interceptor.postExecution(parseResult, commandResult);
-          if (postExecResult != null) {
-            if (Status.ERROR.equals(postExecResult.getStatus())) {
-              if (logWrapper.infoEnabled()) {
-                logWrapper.info("Post execution Result :: "+ResultBuilder.resultAsString(postExecResult));
-              }
-            } else if (logWrapper.fineEnabled()) {
-              logWrapper.fine("Post execution Result :: "+ResultBuilder.resultAsString(postExecResult));
-            }
-            commandResult = postExecResult;
+    }
+
+    if (response == null) {
+      shell.logWarning("Response was null for: \"" + parseResult.getUserInput() + "\". (gfsh.isConnected="
+ shell.isConnectedAndReady() + ")", null);
+      return ResultBuilder.createBadResponseErrorResult(" Error occurred while " +
+        "executing \"" + parseResult.getUserInput() + "\" on manager. " +
+        "Please check manager logs for error.");
+    }
+
+    if (logWrapper.fineEnabled()) {
+      logWrapper.fine("Received response :: "+response);
+    }
+    CommandResponse commandResponse = CommandResponseBuilder.prepareCommandResponseFromJson((String)
response);
+
+    if (commandResponse.isFailedToPersist()) {
+      shell.printAsSevere(CliStrings.SHARED_CONFIGURATION_FAILED_TO_PERSIST_COMMAND_CHANGES);
+      logWrapper.severe(CliStrings.SHARED_CONFIGURATION_FAILED_TO_PERSIST_COMMAND_CHANGES);
+    }
+
+    String debugInfo = commandResponse.getDebugInfo();
+    if (debugInfo != null && !debugInfo.trim().isEmpty()) {
+      //TODO - Abhishek When debug is ON, log response in gfsh logs
+      //TODO - Abhishek handle \n better. Is it coming from GemFire formatter
+      debugInfo = debugInfo.replaceAll("\n\n\n", "\n");
+      debugInfo = debugInfo.replaceAll("\n\n", "\n");
+      debugInfo = debugInfo.replaceAll("\n", "\n[From Manager : "+commandResponse.getSender()+"]");
+      debugInfo = "[From Manager : "+commandResponse.getSender()+"]" + debugInfo;
+      LogWrapper.getInstance().info(debugInfo);
+    }
+    commandResult = ResultBuilder.fromJson((String) response);
+
+    //3. Post Remote Execution
+    if (interceptor != null) {
+      Result postExecResult = interceptor.postExecution(parseResult, commandResult);
+      if (postExecResult != null) {
+        if (Status.ERROR.equals(postExecResult.getStatus())) {
+          if (logWrapper.infoEnabled()) {
+            logWrapper.info("Post execution Result :: "+ResultBuilder.resultAsString(postExecResult));
           }
+        } else if (logWrapper.fineEnabled()) {
+          logWrapper.fine("Post execution Result :: "+ResultBuilder.resultAsString(postExecResult));
         }
-      }// not null response
-    } else {
-      shell.logWarning("Can't execute a remote command without connection. Use 'connect'
first to connect.", null);
-      logWrapper.info("Can't execute a remote command \""+parseResult.getUserInput()+"\"
without connection. Use 'connect' first to connect to GemFire.");
+        commandResult = postExecResult;
+      }
     }
+
     return commandResult;
   }
 }

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/14a548ff/geode-core/src/main/java/com/gemstone/gemfire/management/internal/security/ResourceOperationContext.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/security/ResourceOperationContext.java
b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/security/ResourceOperationContext.java
index ab49270..580b6c0 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/security/ResourceOperationContext.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/security/ResourceOperationContext.java
@@ -46,7 +46,7 @@ public class ResourceOperationContext extends OperationContext {
     //for DATA resource, when we construct the lock to guard the operations, there should
always be a 3rd part (regionName),
     // if no regionName is specified, we need to add "NULL" to it.
     // this means, for general data operations, or operations that we can't put a regionName
on yet, like backup diskstore, query data, create regions
-    // it will require DATA:REAT/WRITE:NULL role
+    // it will require DATA:READ/WRITE:NULL role
     if(this.resource==Resource.DATA && this.regionName==null){
       this.regionName = "NULL";
     }

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/14a548ff/geode-core/src/main/java/com/gemstone/gemfire/management/internal/web/controllers/AbstractCommandsController.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/web/controllers/AbstractCommandsController.java
b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/web/controllers/AbstractCommandsController.java
index c411972..1f6c52a 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/web/controllers/AbstractCommandsController.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/web/controllers/AbstractCommandsController.java
@@ -38,6 +38,7 @@ import com.gemstone.gemfire.internal.cache.GemFireCacheImpl;
 import com.gemstone.gemfire.internal.lang.StringUtils;
 import com.gemstone.gemfire.internal.logging.LogService;
 import com.gemstone.gemfire.internal.logging.log4j.LogMarker;
+import com.gemstone.gemfire.internal.security.GeodeSecurityUtil;
 import com.gemstone.gemfire.internal.util.ArrayUtils;
 import com.gemstone.gemfire.management.DistributedSystemMXBean;
 import com.gemstone.gemfire.management.ManagementService;
@@ -48,9 +49,8 @@ import com.gemstone.gemfire.management.internal.SystemManagementService;
 import com.gemstone.gemfire.management.internal.cli.shell.Gfsh;
 import com.gemstone.gemfire.management.internal.cli.util.CommandStringBuilder;
 import com.gemstone.gemfire.management.internal.web.controllers.support.LoginHandlerInterceptor;
-import com.gemstone.gemfire.management.internal.web.controllers.support.MemberMXBeanAdapter;
 import com.gemstone.gemfire.management.internal.web.util.UriUtils;
-import com.gemstone.gemfire.internal.security.GeodeSecurityUtil;
+import com.gemstone.gemfire.security.NotAuthorizedException;
 
 import org.apache.logging.log4j.Logger;
 import org.springframework.beans.propertyeditors.StringArrayPropertyEditor;
@@ -425,7 +425,6 @@ public abstract class AbstractCommandsController {
    * 
    * @return a proxy instance to the MemberMXBean of the GemFire Manager.
    * @see #getMBeanServer()
-   * @see #createMemberMXBeanForManagerUsingAdapter(javax.management.MBeanServer, javax.management.ObjectName)
    * @see #createMemberMXBeanForManagerUsingProxy(javax.management.MBeanServer, javax.management.ObjectName)
    * @see com.gemstone.gemfire.management.DistributedSystemMXBean
    * @see com.gemstone.gemfire.management.MemberMXBean
@@ -456,21 +455,6 @@ public abstract class AbstractCommandsController {
   }
 
   /**
-   * Creates an Adapter using the Platform MBeanServer and ObjectName to invoke operations
on the GemFire Manager's
-   * MemberMXBean.
-   * 
-   * @param server a reference to this JVM's Platform MBeanServer.
-   * @param managingMemberObjectName the ObjectName of the GemFire Manager's MemberMXBean
registered in
-   * the Platform MBeanServer.
-   * @return an Adapter for invoking operations on the GemFire Manager's MemberMXBean.
-   * @see com.gemstone.gemfire.management.internal.web.controllers.AbstractCommandsController.MemberMXBeanProxy
-   * @see #createMemberMXBeanForManagerUsingProxy(javax.management.MBeanServer, javax.management.ObjectName)
-   */
-  private MemberMXBean createMemberMXBeanForManagerUsingAdapter(final MBeanServer server,
final ObjectName managingMemberObjectName) {
-    return new MemberMXBeanProxy(server, managingMemberObjectName);
-  }
-
-  /**
    * Creates a Proxy using the Platform MBeanServer and ObjectName in order to access attributes
and invoke operations
    * on the GemFire Manager's MemberMXBean.
    * 
@@ -478,7 +462,6 @@ public abstract class AbstractCommandsController {
    * @param managingMemberObjectName the ObjectName of the GemFire Manager's MemberMXBean
registered in
    * the Platform MBeanServer.
    * @return a Proxy for accessing attributes and invoking operations on the GemFire Manager's
MemberMXBean.
-   * @see #createMemberMXBeanForManagerUsingAdapter(javax.management.MBeanServer, javax.management.ObjectName)
    * @see javax.management.JMX#newMXBeanProxy(javax.management.MBeanServerConnection, javax.management.ObjectName,
Class)
    */
   private MemberMXBean createMemberMXBeanForManagerUsingProxy(final MBeanServer server, final
ObjectName managingMemberObjectName) {
@@ -538,7 +521,7 @@ public abstract class AbstractCommandsController {
   /**
    * Executes the specified command as entered by the user using the GemFire Shell (Gfsh).
 Note, Gfsh performs
    * validation of the command during parsing before sending the command to the Manager for
processing.
-   * 
+   *
    * @param command a String value containing a valid command String as would be entered
by the user in Gfsh.
    * @return a result of the command execution as a String, typically marshalled in JSON
to be serialized back to Gfsh.
    * @see com.gemstone.gemfire.management.internal.cli.shell.Gfsh
@@ -549,14 +532,26 @@ public abstract class AbstractCommandsController {
   protected String processCommand(final String command) {
     return processCommand(command, getEnvironment(), null);
   }
+
   protected Callable<ResponseEntity<String>> getProcessCommandCallable(final
String command){
     return getProcessCommandCallable(command, null);
   }
 
   protected Callable<ResponseEntity<String>> getProcessCommandCallable(final
String command, final byte[][] fileData){
     Callable callable = new Callable<ResponseEntity<String>>() {
-      @Override public ResponseEntity<String> call() throws Exception {
-        return new ResponseEntity<String>(processCommand(command, fileData), HttpStatus.OK);
+      @Override
+      public ResponseEntity<String> call() throws Exception {
+        String result = null;
+        try {
+          result = processCommand(command, fileData);
+        }
+        catch(NotAuthorizedException ex){
+          return new ResponseEntity<String>(ex.getMessage(), HttpStatus.FORBIDDEN);
+        }
+        catch(Exception ex){
+          return new ResponseEntity<String>(ex.getMessage(), HttpStatus.INTERNAL_SERVER_ERROR);
+        }
+        return new ResponseEntity<String>(result, HttpStatus.OK);
       }
     };
     return GeodeSecurityUtil.associateWith(callable);
@@ -614,52 +609,13 @@ public abstract class AbstractCommandsController {
    */
   protected String processCommand(final String command, final Map<String, String> environment,
final byte[][] fileData) {
     logger.info(LogMarker.CONFIG, "Processing Command ({}) with Environment ({}) having File
Data ({})...", command,
-        environment, (fileData != null));
-    String result =  getManagingMemberMXBean().processCommand(command, environment, ArrayUtils.toByteArray(fileData));
-
-    return result;
+      environment, (fileData != null));
+    return getManagingMemberMXBean().processCommand(command, environment, ArrayUtils.toByteArray(fileData));
   }
 
-
-  /**
-   * The MemberMXBeanProxy class is a proxy for the MemberMXBean interface transforming an
operation on the member
-   * MBean into a invocation on the MBeanServer, invoke method.
-   * 
-   * @see com.gemstone.gemfire.management.internal.web.controllers.support.MemberMXBeanAdapter
-   */
-  private static class MemberMXBeanProxy extends MemberMXBeanAdapter {
-
-    private final MBeanServer server;
-
-    private final ObjectName objectName;
-
-    public MemberMXBeanProxy(final MBeanServer server, final ObjectName objectName) {
-      assertNotNull(server, "The connection or reference to the Platform MBeanServer cannot
be null!");
-      assertNotNull(objectName, "The JMX ObjectName for the GemFire Manager MemberMXBean
cannot be null!");
-      this.server = server;
-      this.objectName = objectName;
-    }
-
-    protected MBeanServer getMBeanServer() {
-      return server;
-    }
-
-    protected ObjectName getObjectName() {
-      return objectName;
-    }
-
-    @Override
-    public String processCommand(final String commandString, final Map<String, String>
env) {
-      try {
-        return String.valueOf(getMBeanServer().invoke(getObjectName(), "processCommand",
-          new Object[] { commandString, env }, new String[] { String.class.getName(), Map.class.getName()
}));
-      }
-      catch (Exception e) {
-        throw new RuntimeException(String.format(
-          "An error occurred while executing processCommand with command String (%1$s) on
the MemberMXBean (%2$s) of the GemFire Manager using environment (%3$s)!",
-            commandString, getObjectName(), env), e);
-      }
-    }
+  @ExceptionHandler(NotAuthorizedException.class)
+  public ResponseEntity<String> handleAppException(NotAuthorizedException ex) {
+    return new ResponseEntity<String>(ex.getMessage(), HttpStatus.FORBIDDEN);
   }
 
 }

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/14a548ff/geode-core/src/main/java/com/gemstone/gemfire/management/internal/web/shell/AbstractHttpOperationInvoker.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/web/shell/AbstractHttpOperationInvoker.java
b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/web/shell/AbstractHttpOperationInvoker.java
index b2159d2..944644f 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/management/internal/web/shell/AbstractHttpOperationInvoker.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/management/internal/web/shell/AbstractHttpOperationInvoker.java
@@ -42,7 +42,6 @@ import com.gemstone.gemfire.management.DistributedSystemMXBean;
 import com.gemstone.gemfire.management.internal.MBeanJMXAdapter;
 import com.gemstone.gemfire.management.internal.ManagementConstants;
 import com.gemstone.gemfire.management.internal.cli.shell.Gfsh;
-import com.gemstone.gemfire.management.internal.security.ResourceConstants;
 import com.gemstone.gemfire.management.internal.web.domain.Link;
 import com.gemstone.gemfire.management.internal.web.domain.QueryParameterSource;
 import com.gemstone.gemfire.management.internal.web.http.ClientHttpRequest;
@@ -51,6 +50,7 @@ import com.gemstone.gemfire.management.internal.web.http.HttpMethod;
 import com.gemstone.gemfire.management.internal.web.http.converter.SerializableObjectHttpMessageConverter;
 import com.gemstone.gemfire.management.internal.web.shell.support.HttpMBeanProxyFactory;
 import com.gemstone.gemfire.management.internal.web.util.UriUtils;
+import com.gemstone.gemfire.security.NotAuthorizedException;
 
 import org.apache.logging.log4j.Logger;
 import org.springframework.http.HttpStatus;
@@ -206,15 +206,17 @@ public abstract class AbstractHttpOperationInvoker implements HttpOperationInvok
 
       @Override
       public void handleError(final ClientHttpResponse response) throws IOException {
-        final String message = String.format("The HTTP request failed with: %1$d - %2$s",
response.getRawStatusCode(),
-          response.getStatusText());
-
-        //gfsh.logSevere(message, null);
+        String body = readBody(response);
+        final String message = String.format("The HTTP request failed with: %1$d - %2$s.",
response.getRawStatusCode(), body);
 
         if (gfsh.getDebug()) {
-          gfsh.logSevere(readBody(response), null);
+          gfsh.logSevere(body, null);
         }
-        throw new RuntimeException(message);
+
+        if(response.getRawStatusCode()==403)
+          throw new NotAuthorizedException(message);
+        else
+          throw new RuntimeException(message);
       }
 
       private String readBody(final ClientHttpResponse response) throws IOException {

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/14a548ff/geode-core/src/test/java/com/gemstone/gemfire/management/internal/security/GfshCommandsSecurityTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/com/gemstone/gemfire/management/internal/security/GfshCommandsSecurityTest.java
b/geode-core/src/test/java/com/gemstone/gemfire/management/internal/security/GfshCommandsSecurityTest.java
index 377ab77..b21302e 100644
--- a/geode-core/src/test/java/com/gemstone/gemfire/management/internal/security/GfshCommandsSecurityTest.java
+++ b/geode-core/src/test/java/com/gemstone/gemfire/management/internal/security/GfshCommandsSecurityTest.java
@@ -112,9 +112,11 @@ public class GfshCommandsSecurityTest {
 
 
   private void runCommandsWithAndWithout(String permission) throws Exception{
-    List<TestCommand> permitted = TestCommand.getPermittedCommands(new WildcardPermission(permission,
true));
-    for(TestCommand clusterRead:permitted) {
-      LogService.getLogger().info("Processing authorized command: "+clusterRead.getCommand());gfsh.executeCommand(clusterRead.getCommand());
+    List<TestCommand> allPermitted = TestCommand.getPermittedCommands(new WildcardPermission(permission,
true));
+    for(TestCommand permitted:allPermitted) {
+      LogService.getLogger().info("Processing authorized command: "+permitted.getCommand());
+
+      gfsh.executeCommand(permitted.getCommand());
       CommandResult result = (CommandResult) gfsh.getResult();
       assertNotNull(result);
 
@@ -127,7 +129,7 @@ public class GfshCommandsSecurityTest {
     }
 
     List<TestCommand> others = TestCommand.getCommands();
-    others.removeAll(permitted);
+    others.removeAll(allPermitted);
     for(TestCommand other:others) {
       // skip no permission commands
       if(other.getPermission()==null)
@@ -135,6 +137,7 @@ public class GfshCommandsSecurityTest {
 
       LogService.getLogger().info("Processing unauthorized command: "+other.getCommand());
       gfsh.executeCommand(other.getCommand());
+
       CommandResult result = (CommandResult) gfsh.getResult();
       int errorCode = ((ErrorResultData) result.getResultData()).getErrorCode();
 

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/14a548ff/geode-core/src/test/java/com/gemstone/gemfire/management/internal/security/MemberMBeanSecurityJUnitTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/com/gemstone/gemfire/management/internal/security/MemberMBeanSecurityJUnitTest.java
b/geode-core/src/test/java/com/gemstone/gemfire/management/internal/security/MemberMBeanSecurityJUnitTest.java
index 8261d09..cabf555 100644
--- a/geode-core/src/test/java/com/gemstone/gemfire/management/internal/security/MemberMBeanSecurityJUnitTest.java
+++ b/geode-core/src/test/java/com/gemstone/gemfire/management/internal/security/MemberMBeanSecurityJUnitTest.java
@@ -103,7 +103,7 @@ public class MemberMBeanSecurityJUnitTest {
     assertThatThrownBy(() -> bean.isCacheServer()).hasMessageContaining(TestCommand.clusterRead.toString());
     assertThatThrownBy(() -> bean.isServer()).hasMessageContaining(TestCommand.clusterRead.toString());
     assertThatThrownBy(() -> bean.listConnectedGatewayReceivers()).hasMessageContaining(TestCommand.clusterRead.toString());
-    //assertThatThrownBy(() -> bean.processCommand("create region --name=Region_A")).hasMessageContaining("DATA:MANAGE");
+    assertThatThrownBy(() -> bean.processCommand("create region --name=Region_A")).hasMessageContaining("DATA:MANAGE");
     assertThatThrownBy(() -> bean.showJVMMetrics()).hasMessageContaining(TestCommand.clusterRead.toString());
     assertThatThrownBy(() -> bean.status()).hasMessageContaining(TestCommand.clusterRead.toString());
   }

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/14a548ff/geode-core/src/test/java/com/gemstone/gemfire/management/internal/web/controllers/WanCommandsControllerJUnitTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/com/gemstone/gemfire/management/internal/web/controllers/WanCommandsControllerJUnitTest.java
b/geode-core/src/test/java/com/gemstone/gemfire/management/internal/web/controllers/WanCommandsControllerJUnitTest.java
index 03d39fd..2e1aa65 100755
--- a/geode-core/src/test/java/com/gemstone/gemfire/management/internal/web/controllers/WanCommandsControllerJUnitTest.java
+++ b/geode-core/src/test/java/com/gemstone/gemfire/management/internal/web/controllers/WanCommandsControllerJUnitTest.java
@@ -20,14 +20,15 @@ import static com.gemstone.gemfire.management.internal.cli.i18n.CliStrings.*;
 import static junitparams.JUnitParamsRunner.*;
 import static org.assertj.core.api.Assertions.*;
 
-import junitparams.JUnitParamsRunner;
-import junitparams.Parameters;
+import com.gemstone.gemfire.test.junit.categories.UnitTest;
+
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 import org.junit.runner.RunWith;
 
-import com.gemstone.gemfire.test.junit.categories.UnitTest;
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
 
 /**
  * Unit tests for WanCommandsController. 



Mime
View raw message