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) <[email protected]>
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);
