storm-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From etha...@apache.org
Subject [storm] branch master updated: [STORM-3631] Fix possible ClassCastException and NullPointerException caused by configs like logs.users/groups
Date Mon, 04 May 2020 19:58:44 GMT
This is an automated email from the ASF dual-hosted git repository.

ethanli pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/storm.git


The following commit(s) were added to refs/heads/master by this push:
     new 0553e9a  [STORM-3631] Fix possible ClassCastException and NullPointerException caused
by configs like logs.users/groups
     new 06db0e9  Merge pull request #3262 from Ethanlm/STORM-3631
0553e9a is described below

commit 0553e9a605d46addc8e01f021ad651296d2e4e8d
Author: Meng Li (Ethan) <ethanopensource@gmail.com>
AuthorDate: Fri May 1 11:17:00 2020 -0500

    [STORM-3631] Fix possible ClassCastException and NullPointerException caused by configs
like logs.users/groups
---
 storm-client/src/jvm/org/apache/storm/Config.java  |  8 ++--
 .../auth/authorizer/SimpleACLAuthorizer.java       |  7 ++--
 .../authorizer/SupervisorSimpleACLAuthorizer.java  |  7 ++--
 .../jvm/org/apache/storm/utils/ObjectReader.java   | 13 ++++--
 .../main/java/org/apache/storm/DaemonConfig.java   |  4 +-
 .../org/apache/storm/daemon/nimbus/Nimbus.java     |  3 +-
 .../apache/storm/daemon/supervisor/Container.java  | 21 ++++------
 .../org/apache/storm/utils/ServerConfigUtils.java  | 46 +++++-----------------
 .../storm/daemon/supervisor/ContainerTest.java     |  5 ++-
 9 files changed, 45 insertions(+), 69 deletions(-)

diff --git a/storm-client/src/jvm/org/apache/storm/Config.java b/storm-client/src/jvm/org/apache/storm/Config.java
index 64045a0..3316f42 100644
--- a/storm-client/src/jvm/org/apache/storm/Config.java
+++ b/storm-client/src/jvm/org/apache/storm/Config.java
@@ -141,25 +141,25 @@ public class Config extends HashMap<String, Object> {
      * A list of users that are allowed to interact with the topology.  To use this set nimbus.authorizer
to
      * org.apache.storm.security.auth.authorizer.SimpleACLAuthorizer
      */
-    @IsStringList
+    @IsStringOrStringList
     public static final String TOPOLOGY_USERS = "topology.users";
     /**
      * A list of groups that are allowed to interact with the topology.  To use this set
nimbus.authorizer to
      * org.apache.storm.security.auth.authorizer.SimpleACLAuthorizer
      */
-    @IsStringList
+    @IsStringOrStringList
     public static final String TOPOLOGY_GROUPS = "topology.groups";
     /**
      * A list of readonly users that are allowed to interact with the topology.  To use this
set nimbus.authorizer to
      * org.apache.storm.security.auth.authorizer.SimpleACLAuthorizer
      */
-    @IsStringList
+    @IsStringOrStringList
     public static final String TOPOLOGY_READONLY_USERS = "topology.readonly.users";
     /**
      * A list of readonly groups that are allowed to interact with the topology.  To use
this set nimbus.authorizer to
      * org.apache.storm.security.auth.authorizer.SimpleACLAuthorizer
      */
-    @IsStringList
+    @IsStringOrStringList
     public static final String TOPOLOGY_READONLY_GROUPS = "topology.readonly.groups";
     /**
      * True if Storm should timeout messages or not. Defaults to true. This is meant to be
used in unit tests to prevent tuples from being
diff --git a/storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java
b/storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java
index f639b28..3650c5d 100644
--- a/storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java
+++ b/storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java
@@ -24,6 +24,7 @@ import org.apache.storm.security.auth.IAuthorizer;
 import org.apache.storm.security.auth.IGroupMappingServiceProvider;
 import org.apache.storm.security.auth.IPrincipalToLocal;
 import org.apache.storm.security.auth.ReqContext;
+import org.apache.storm.utils.ObjectReader;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -173,7 +174,7 @@ public class SimpleACLAuthorizer implements IAuthorizer {
         Set<String> configuredUsers = new HashSet<>();
 
         if (topoConf.containsKey(userConfigKey)) {
-            configuredUsers.addAll((Collection<String>) topoConf.get(userConfigKey));
+            configuredUsers.addAll(ObjectReader.getStrings(topoConf.get(userConfigKey)));
         }
 
         if (configuredUsers.contains(principal) || configuredUsers.contains(user)) {
@@ -181,8 +182,8 @@ public class SimpleACLAuthorizer implements IAuthorizer {
         }
 
         Set<String> configuredGroups = new HashSet<>();
-        if (topoConf.containsKey(groupConfigKey) && topoConf.get(groupConfigKey)
!= null) {
-            configuredGroups.addAll((Collection<String>) topoConf.get(groupConfigKey));
+        if (topoConf.containsKey(groupConfigKey)) {
+            configuredGroups.addAll(ObjectReader.getStrings(topoConf.get(groupConfigKey)));
         }
 
         return checkUserGroupAllowed(userGroups, configuredGroups);
diff --git a/storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SupervisorSimpleACLAuthorizer.java
b/storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SupervisorSimpleACLAuthorizer.java
index 5875ff0..9358e55 100644
--- a/storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SupervisorSimpleACLAuthorizer.java
+++ b/storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SupervisorSimpleACLAuthorizer.java
@@ -24,6 +24,7 @@ import org.apache.storm.security.auth.IAuthorizer;
 import org.apache.storm.security.auth.IGroupMappingServiceProvider;
 import org.apache.storm.security.auth.IPrincipalToLocal;
 import org.apache.storm.security.auth.ReqContext;
+import org.apache.storm.utils.ObjectReader;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -125,7 +126,7 @@ public class SupervisorSimpleACLAuthorizer implements IAuthorizer {
         Set<String> configuredUsers = new HashSet<>();
 
         if (topoConf.containsKey(userConfigKey)) {
-            configuredUsers.addAll((Collection<String>) topoConf.get(userConfigKey));
+            configuredUsers.addAll(ObjectReader.getStrings(topoConf.get(userConfigKey)));
         }
 
         if (configuredUsers.contains(principal) || configuredUsers.contains(user)) {
@@ -133,8 +134,8 @@ public class SupervisorSimpleACLAuthorizer implements IAuthorizer {
         }
 
         Set<String> configuredGroups = new HashSet<>();
-        if (topoConf.containsKey(groupConfigKey) && topoConf.get(groupConfigKey)
!= null) {
-            configuredGroups.addAll((Collection<String>) topoConf.get(groupConfigKey));
+        if (topoConf.containsKey(groupConfigKey)) {
+            configuredGroups.addAll(ObjectReader.getStrings(topoConf.get(groupConfigKey)));
         }
 
         return checkUserGroupAllowed(userGroups, configuredGroups);
diff --git a/storm-client/src/jvm/org/apache/storm/utils/ObjectReader.java b/storm-client/src/jvm/org/apache/storm/utils/ObjectReader.java
index 533f546..54445b5 100644
--- a/storm-client/src/jvm/org/apache/storm/utils/ObjectReader.java
+++ b/storm-client/src/jvm/org/apache/storm/utils/ObjectReader.java
@@ -24,9 +24,14 @@ import java.util.List;
 
 public class ObjectReader {
 
+    /**
+     * Convert the input into a list of string; ignore null members.
+     * @param o the input object
+     * @return a list of string
+     */
     public static List<String> getStrings(final Object o) {
         if (o == null) {
-            return new ArrayList<String>();
+            return new ArrayList<>();
         } else if (o instanceof String) {
             return new ArrayList<String>() {
                 {
@@ -34,9 +39,11 @@ public class ObjectReader {
                 }
             };
         } else if (o instanceof Collection) {
-            List<String> answer = new ArrayList<String>();
+            List<String> answer = new ArrayList<>();
             for (Object v : (Collection) o) {
-                answer.add(v.toString());
+                if (v != null) {
+                    answer.add(v.toString());
+                }
             }
             return answer;
         } else {
diff --git a/storm-server/src/main/java/org/apache/storm/DaemonConfig.java b/storm-server/src/main/java/org/apache/storm/DaemonConfig.java
index 4d186aa..0918a72 100644
--- a/storm-server/src/main/java/org/apache/storm/DaemonConfig.java
+++ b/storm-server/src/main/java/org/apache/storm/DaemonConfig.java
@@ -485,13 +485,13 @@ public class DaemonConfig implements Validated {
     /**
      * A list of users allowed to view logs via the Log Viewer.
      */
-    @IsStringList
+    @IsStringOrStringList
     public static final String LOGS_USERS = "logs.users";
 
     /**
      * A list of groups allowed to view logs via the Log Viewer.
      */
-    @IsStringList
+    @IsStringOrStringList
     public static final String LOGS_GROUPS = "logs.groups";
 
     /**
diff --git a/storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java b/storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
index d01e223..db548bc 100644
--- a/storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
+++ b/storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
@@ -3145,8 +3145,7 @@ public class Nimbus implements Iface, Shutdownable, DaemonCommon {
             ReqContext req = ReqContext.context();
             Principal principal = req.principal();
             String submitterPrincipal = principal == null ? null : principal.toString();
-            @SuppressWarnings("unchecked")
-            Set<String> topoAcl = new HashSet<>((List<String>) topoConf.getOrDefault(Config.TOPOLOGY_USERS,
Collections.emptyList()));
+            Set<String> topoAcl = new HashSet<>(ObjectReader.getStrings(topoConf.get(Config.TOPOLOGY_USERS)));
             topoAcl.add(submitterPrincipal);
             String submitterUser = principalToLocal.toLocal(principal);
             topoAcl.add(submitterUser);
diff --git a/storm-server/src/main/java/org/apache/storm/daemon/supervisor/Container.java
b/storm-server/src/main/java/org/apache/storm/daemon/supervisor/Container.java
index c09b6f2..966ea54 100644
--- a/storm-server/src/main/java/org/apache/storm/daemon/supervisor/Container.java
+++ b/storm-server/src/main/java/org/apache/storm/daemon/supervisor/Container.java
@@ -386,7 +386,6 @@ public abstract class Container implements Killable {
      * @param user the user this is going to run as
      * @throws IOException on any error
      */
-    @SuppressWarnings("unchecked")
     protected void writeLogMetadata(String user) throws IOException {
         type.assertFull();
         Map<String, Object> data = new HashMap<>();
@@ -395,29 +394,23 @@ public abstract class Container implements Killable {
 
         Set<String> logsGroups = new HashSet<>();
         if (topoConf.get(DaemonConfig.LOGS_GROUPS) != null) {
-            List<String> groups = (List<String>) topoConf.get(DaemonConfig.LOGS_GROUPS);
-            for (String group : groups) {
-                logsGroups.add(group);
-            }
+            List<String> groups = ObjectReader.getStrings(topoConf.get(DaemonConfig.LOGS_GROUPS));
+            logsGroups.addAll(groups);
         }
         if (topoConf.get(Config.TOPOLOGY_GROUPS) != null) {
-            List<String> topGroups = (List<String>) topoConf.get(Config.TOPOLOGY_GROUPS);
+            List<String> topGroups = ObjectReader.getStrings(topoConf.get(Config.TOPOLOGY_GROUPS));
             logsGroups.addAll(topGroups);
         }
         data.put(DaemonConfig.LOGS_GROUPS, logsGroups.toArray());
 
         Set<String> logsUsers = new HashSet<>();
         if (topoConf.get(DaemonConfig.LOGS_USERS) != null) {
-            List<String> logUsers = (List<String>) topoConf.get(DaemonConfig.LOGS_USERS);
-            for (String logUser : logUsers) {
-                logsUsers.add(logUser);
-            }
+            List<String> logUsers = ObjectReader.getStrings(topoConf.get(DaemonConfig.LOGS_USERS));
+            logsUsers.addAll(logUsers);
         }
         if (topoConf.get(Config.TOPOLOGY_USERS) != null) {
-            List<String> topUsers = (List<String>) topoConf.get(Config.TOPOLOGY_USERS);
-            for (String logUser : topUsers) {
-                logsUsers.add(logUser);
-            }
+            List<String> topUsers = ObjectReader.getStrings(topoConf.get(Config.TOPOLOGY_USERS));
+            logsUsers.addAll(topUsers);
         }
         data.put(DaemonConfig.LOGS_USERS, logsUsers.toArray());
 
diff --git a/storm-server/src/main/java/org/apache/storm/utils/ServerConfigUtils.java b/storm-server/src/main/java/org/apache/storm/utils/ServerConfigUtils.java
index 2c932b8..156bf20 100644
--- a/storm-server/src/main/java/org/apache/storm/utils/ServerConfigUtils.java
+++ b/storm-server/src/main/java/org/apache/storm/utils/ServerConfigUtils.java
@@ -87,47 +87,21 @@ public class ServerConfigUtils {
 
     /* TODO: make sure test these two functions in manual tests */
     public static List<String> getTopoLogsUsers(Map<String, Object> topologyConf)
{
-        List<String> logsUsers = (List<String>) topologyConf.get(DaemonConfig.LOGS_USERS);
-        List<String> topologyUsers = (List<String>) topologyConf.get(Config.TOPOLOGY_USERS);
-        Set<String> mergedUsers = new HashSet<String>();
-        if (logsUsers != null) {
-            for (String user : logsUsers) {
-                if (user != null) {
-                    mergedUsers.add(user);
-                }
-            }
-        }
-        if (topologyUsers != null) {
-            for (String user : topologyUsers) {
-                if (user != null) {
-                    mergedUsers.add(user);
-                }
-            }
-        }
-        List<String> ret = new ArrayList<String>(mergedUsers);
+        List<String> logsUsers = ObjectReader.getStrings(topologyConf.get(DaemonConfig.LOGS_USERS));
+        List<String> topologyUsers = ObjectReader.getStrings(topologyConf.get(Config.TOPOLOGY_USERS));
+        Set<String> mergedUsers = new HashSet<>(logsUsers);
+        mergedUsers.addAll(topologyUsers);
+        List<String> ret = new ArrayList<>(mergedUsers);
         Collections.sort(ret);
         return ret;
     }
 
     public static List<String> getTopoLogsGroups(Map<String, Object> topologyConf)
{
-        List<String> logsGroups = (List<String>) topologyConf.get(DaemonConfig.LOGS_GROUPS);
-        List<String> topologyGroups = (List<String>) topologyConf.get(Config.TOPOLOGY_GROUPS);
-        Set<String> mergedGroups = new HashSet<String>();
-        if (logsGroups != null) {
-            for (String group : logsGroups) {
-                if (group != null) {
-                    mergedGroups.add(group);
-                }
-            }
-        }
-        if (topologyGroups != null) {
-            for (String group : topologyGroups) {
-                if (group != null) {
-                    mergedGroups.add(group);
-                }
-            }
-        }
-        List<String> ret = new ArrayList<String>(mergedGroups);
+        List<String> logsGroups = ObjectReader.getStrings(topologyConf.get(DaemonConfig.LOGS_GROUPS));
+        List<String> topologyGroups = ObjectReader.getStrings(topologyConf.get(Config.TOPOLOGY_GROUPS));
+        Set<String> mergedGroups = new HashSet<>(logsGroups);
+        mergedGroups.addAll(topologyGroups);
+        List<String> ret = new ArrayList<>(mergedGroups);
         Collections.sort(ret);
         return ret;
     }
diff --git a/storm-server/src/test/java/org/apache/storm/daemon/supervisor/ContainerTest.java
b/storm-server/src/test/java/org/apache/storm/daemon/supervisor/ContainerTest.java
index cf5ec8d..198d03f 100644
--- a/storm-server/src/test/java/org/apache/storm/daemon/supervisor/ContainerTest.java
+++ b/storm-server/src/test/java/org/apache/storm/daemon/supervisor/ContainerTest.java
@@ -30,6 +30,7 @@ import org.apache.storm.container.ResourceIsolationInterface;
 import org.apache.storm.daemon.supervisor.Container.ContainerType;
 import org.apache.storm.generated.LocalAssignment;
 import org.apache.storm.generated.ProfileRequest;
+import org.apache.storm.utils.ObjectReader;
 import org.junit.Test;
 import org.yaml.snakeyaml.Yaml;
 
@@ -156,11 +157,11 @@ public class ContainerTest {
         assertEquals(user, result.get(Config.TOPOLOGY_SUBMITTER_USER));
         HashSet<String> allowedUsers = new HashSet<>(topoUsers);
         allowedUsers.addAll(logUsers);
-        assertEquals(allowedUsers, new HashSet<String>((List<String>) result.get(DaemonConfig.LOGS_USERS)));
+        assertEquals(allowedUsers, new HashSet<>(ObjectReader.getStrings(result.get(DaemonConfig.LOGS_USERS))));
 
         HashSet<String> allowedGroups = new HashSet<>(topoGroups);
         allowedGroups.addAll(logGroups);
-        assertEquals(allowedGroups, new HashSet<String>((List<String>) result.get(DaemonConfig.LOGS_GROUPS)));
+        assertEquals(allowedGroups, new HashSet<>(ObjectReader.getStrings(result.get(DaemonConfig.LOGS_GROUPS))));
 
         //Save the current user to help with recovery
         verify(ops).dump(workerUserFile, user);


Mime
View raw message