On Monday, February 2, 2015, Tim Williams <[email protected]> wrote:
> On Mon, Feb 2, 2015 at 8:36 PM, <[email protected] <javascript:;>> > wrote: > > Repository: incubator-blur > > Updated Branches: > > refs/heads/master 4468f6cc5 -> 9e9e8ed1b > > > > > > Changing the server security feature to be driven by a factory pattern. > It also supports a filter pattern for server security. > > > > > > Project: http://git-wip-us.apache.org/repos/asf/incubator-blur/repo > > Commit: > http://git-wip-us.apache.org/repos/asf/incubator-blur/commit/9e9e8ed1 > > Tree: > http://git-wip-us.apache.org/repos/asf/incubator-blur/tree/9e9e8ed1 > > Diff: > http://git-wip-us.apache.org/repos/asf/incubator-blur/diff/9e9e8ed1 > > > > Branch: refs/heads/master > > Commit: 9e9e8ed1b21e4537f3a5bcffe4573f04a8a644f4 > > Parents: 4468f6c > > Author: Aaron McCurry <[email protected] <javascript:;>> > > Authored: Mon Feb 2 20:36:03 2015 -0500 > > Committer: Aaron McCurry <[email protected] <javascript:;>> > > Committed: Mon Feb 2 20:36:03 2015 -0500 > > > > ---------------------------------------------------------------------- > > .../org/apache/blur/server/ServerSecurity.java | 8 +--- > > .../blur/server/ServerSecurityFactory.java | 33 +++++++++++++ > > .../apache/blur/server/ServerSecurityUtil.java | 24 ++++++---- > > .../example/SimpleExampleServerSecurity.java | 24 +++++----- > > .../blur/thrift/ThriftBlurControllerServer.java | 4 +- > > .../blur/thrift/ThriftBlurShardServer.java | 4 +- > > .../org/apache/blur/thrift/ThriftServer.java | 50 > +++++++++++++------- > > .../org/apache/blur/utils/BlurConstants.java | 3 +- > > 8 files changed, 104 insertions(+), 46 deletions(-) > > ---------------------------------------------------------------------- > > > > > > > http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/server/ServerSecurity.java > > ---------------------------------------------------------------------- > > diff --git > a/blur-core/src/main/java/org/apache/blur/server/ServerSecurity.java > b/blur-core/src/main/java/org/apache/blur/server/ServerSecurity.java > > index 3ebf079..ee74559 100644 > > --- a/blur-core/src/main/java/org/apache/blur/server/ServerSecurity.java > > +++ b/blur-core/src/main/java/org/apache/blur/server/ServerSecurity.java > > @@ -19,16 +19,12 @@ package org.apache.blur.server; > > import java.lang.reflect.Method; > > import java.net.InetAddress; > > > > -import org.apache.blur.BlurConfiguration; > > import org.apache.blur.thrift.generated.BlurException; > > import org.apache.blur.user.User; > > > > public abstract class ServerSecurity { > > I didn't mention earlier - this should be named ServerAccessFilter? > > > - > > - public ServerSecurity(BlurConfiguration configuration) { > > - > > - } > > > > - public abstract boolean canAccess(Method method, Object[] args, User > user, InetAddress address, int port) throws BlurException; > > + public abstract boolean canAccess(Method method, Object[] args, User > user, InetAddress address, int port) > > + throws BlurException; > > > > } > > > > > http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/server/ServerSecurityFactory.java > > ---------------------------------------------------------------------- > > diff --git > a/blur-core/src/main/java/org/apache/blur/server/ServerSecurityFactory.java > b/blur-core/src/main/java/org/apache/blur/server/ServerSecurityFactory.java > > new file mode 100644 > > index 0000000..759112e > > --- /dev/null > > +++ > b/blur-core/src/main/java/org/apache/blur/server/ServerSecurityFactory.java > > @@ -0,0 +1,33 @@ > > +/** > > + * Licensed to the Apache Software Foundation (ASF) under one or more > > + * contributor license agreements. See the NOTICE file distributed with > > + * this work for additional information regarding copyright ownership. > > + * The ASF licenses this file to You under the Apache License, Version > 2.0 > > + * (the "License"); you may not use this file except in compliance with > > + * the License. You may obtain a copy of the License at > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > + * > > + * Unless required by applicable law or agreed to in writing, software > > + * distributed under the License is distributed on an "AS IS" BASIS, > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > > + * See the License for the specific language governing permissions and > > + * limitations under the License. > > + */ > > +package org.apache.blur.server; > > + > > +import org.apache.blur.BlurConfiguration; > > + > > +public abstract class ServerSecurityFactory { > > Again with naming, ServerAccessFilterFactory? > > > + > > + public enum ServerType { > > + CONTROLLER, SHARD > > + } > > + > > + public ServerSecurityFactory() { > > + > > + } > > + > > + public abstract ServerSecurity getServerSecurity(ServerType server, > BlurConfiguration configuration); > > + > > +} > > > > > http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/server/ServerSecurityUtil.java > > ---------------------------------------------------------------------- > > diff --git > a/blur-core/src/main/java/org/apache/blur/server/ServerSecurityUtil.java > b/blur-core/src/main/java/org/apache/blur/server/ServerSecurityUtil.java > > index 162fe35..cd7deca 100644 > > --- > a/blur-core/src/main/java/org/apache/blur/server/ServerSecurityUtil.java > > +++ > b/blur-core/src/main/java/org/apache/blur/server/ServerSecurityUtil.java > > @@ -22,6 +22,7 @@ import java.lang.reflect.Method; > > import java.lang.reflect.Proxy; > > import java.net.InetAddress; > > import java.net.InetSocketAddress; > > +import java.util.List; > > > > import org.apache.blur.log.Log; > > import org.apache.blur.log.LogFactory; > > @@ -34,12 +35,15 @@ public class ServerSecurityUtil { > > > > private static final Log LOG = > LogFactory.getLog(ServerSecurityUtil.class); > > > > - public static Iface applySecurity(final Iface iface, final > ServerSecurity serverSecurity, final boolean shardServer) { > > - if (serverSecurity == null) { > > + public static Iface applySecurity(final Iface iface, final > List<ServerSecurity> serverSecurityList, > > + final boolean shardServer) { > > + if (serverSecurityList == null || serverSecurityList.isEmpty()) { > > LOG.info("No server security configured."); > > return iface; > > } > > - LOG.info("Server security configured with [{0}] class [{1}].", > serverSecurity, serverSecurity.getClass()); > > + for (ServerSecurity serverSecurity : serverSecurityList) { > > + LOG.info("Server security configured with [{0}] class [{1}].", > serverSecurity, serverSecurity.getClass()); > > + } > > InvocationHandler handler = new InvocationHandler() { > > @Override > > public Object invoke(Object proxy, Method method, Object[] args) > throws Throwable { > > @@ -53,14 +57,16 @@ public class ServerSecurityUtil { > > InetAddress address = remoteSocketAddress.getAddress(); > > int port = remoteSocketAddress.getPort(); > > User user = UserContext.getUser(); > > - if (serverSecurity.canAccess(method, args, user, address, > port)) { > > - try { > > - return method.invoke(iface, args); > > - } catch (InvocationTargetException e) { > > - throw e.getTargetException(); > > + for (ServerSecurity serverSecurity : serverSecurityList) { > > + if (!serverSecurity.canAccess(method, args, user, address, > port)) { > > + throw new BException("ACCESS DENIED for User [{0}] method > [{1}].", user, method.getName()); > > } > > } > > - throw new BException("ACCESS DENIED for User [{0}] method > [{1}].", user, method.getName()); > > + try { > > + return method.invoke(iface, args); > > + } catch (InvocationTargetException e) { > > + throw e.getTargetException(); > > + } > > } > > }; > > return (Iface) Proxy.newProxyInstance(Iface.class.getClassLoader(), > new Class[] { Iface.class }, handler); > > > > > http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/server/example/SimpleExampleServerSecurity.java > > ---------------------------------------------------------------------- > > diff --git > a/blur-core/src/main/java/org/apache/blur/server/example/SimpleExampleServerSecurity.java > b/blur-core/src/main/java/org/apache/blur/server/example/SimpleExampleServerSecurity.java > > index 206c054..e6628ae 100644 > > --- > a/blur-core/src/main/java/org/apache/blur/server/example/SimpleExampleServerSecurity.java > > +++ > b/blur-core/src/main/java/org/apache/blur/server/example/SimpleExampleServerSecurity.java > > @@ -21,24 +21,26 @@ import java.net.InetAddress; > > > > import org.apache.blur.BlurConfiguration; > > import org.apache.blur.server.ServerSecurity; > > +import org.apache.blur.server.ServerSecurityFactory; > > import org.apache.blur.thrift.generated.BlurException; > > import org.apache.blur.user.User; > > > > -public class SimpleExampleServerSecurity extends ServerSecurity { > > - > > - public SimpleExampleServerSecurity(BlurConfiguration configuration) { > > - super(configuration); > > - } > > +public class SimpleExampleServerSecurity extends ServerSecurityFactory { > > > > @Override > > - public boolean canAccess(Method method, Object[] args, User user, > InetAddress address, int port) throws BlurException { > > - if (method.getName().equals("createTable")) { > > - if (user != null && user.getUsername().equals("admin")) { > > + public ServerSecurity getServerSecurity(ServerType server, > BlurConfiguration configuration) { > > + return new ServerSecurity() { > > + @Override > > + public boolean canAccess(Method method, Object[] args, User user, > InetAddress address, int port) throws BlurException { > > + if (method.getName().equals("createTable")) { > > + if (user != null && user.getUsername().equals("admin")) { > > + return true; > > + } > > + return false; > > + } > > return true; > > } > > - return false; > > - } > > - return true; > > + }; > > } > > > > } > > > > > http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurControllerServer.java > > ---------------------------------------------------------------------- > > diff --git > a/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurControllerServer.java > b/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurControllerServer.java > > index 5632b88..2e5bb20 100644 > > --- > a/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurControllerServer.java > > +++ > b/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurControllerServer.java > > @@ -50,6 +50,7 @@ import static > org.apache.blur.utils.BlurConstants.BLUR_ZOOKEEPER_TIMEOUT_DEFAULT > > import static org.apache.blur.utils.BlurUtil.quietClose; > > > > import java.io.File; > > +import java.util.List; > > > > import org.apache.blur.BlurConfiguration; > > import org.apache.blur.command.ControllerCommandManager; > > @@ -65,6 +66,7 @@ import > org.apache.blur.manager.indexserver.BlurServerShutDown.BlurShutdown; > > import org.apache.blur.metrics.ReporterSetup; > > import org.apache.blur.server.ControllerServerEventHandler; > > import org.apache.blur.server.ServerSecurity; > > +import org.apache.blur.server.ServerSecurityFactory; > > import org.apache.blur.server.ServerSecurityUtil; > > import org.apache.blur.thirdparty.thrift_0_9_0.protocol.TJSONProtocol; > > import org.apache.blur.thirdparty.thrift_0_9_0.server.TServlet; > > @@ -184,7 +186,7 @@ public class ThriftBlurControllerServer extends > ThriftServer { > > Trace.setStorage(traceStorage); > > Trace.setNodeName(nodeName); > > > > - ServerSecurity serverSecurity = getServerSecurity(configuration, > false); > > + List<ServerSecurity> serverSecurity = > getServerSecurityList(configuration, > ServerSecurityFactory.ServerType.CONTROLLER); > > > > Iface iface = BlurUtil.wrapFilteredBlurServer(configuration, > controllerServer, false); > > iface = ServerSecurityUtil.applySecurity(iface, serverSecurity, > false); > > > > > http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurShardServer.java > > ---------------------------------------------------------------------- > > diff --git > a/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurShardServer.java > b/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurShardServer.java > > index 0d91bec..5a0549a 100644 > > --- > a/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurShardServer.java > > +++ > b/blur-core/src/main/java/org/apache/blur/thrift/ThriftBlurShardServer.java > > @@ -59,6 +59,7 @@ import static > org.apache.blur.utils.BlurUtil.quietClose; > > import java.io.Closeable; > > import java.io.File; > > import java.lang.reflect.Constructor; > > +import java.util.List; > > import java.util.Map.Entry; > > import java.util.Set; > > import java.util.Timer; > > @@ -85,6 +86,7 @@ import > org.apache.blur.manager.indexserver.DistributedLayoutFactoryImpl; > > import org.apache.blur.metrics.JSONReporter; > > import org.apache.blur.metrics.ReporterSetup; > > import org.apache.blur.server.ServerSecurity; > > +import org.apache.blur.server.ServerSecurityFactory; > > import org.apache.blur.server.ServerSecurityUtil; > > import org.apache.blur.server.ShardServerEventHandler; > > import org.apache.blur.server.TableContext; > > @@ -264,7 +266,7 @@ public class ThriftBlurShardServer extends > ThriftServer { > > Trace.setStorage(traceStorage); > > Trace.setNodeName(nodeName); > > > > - ServerSecurity serverSecurity = getServerSecurity(configuration, > true); > > + List<ServerSecurity> serverSecurity = > getServerSecurityList(configuration, > ServerSecurityFactory.ServerType.SHARD); > > > > Iface iface = BlurUtil.wrapFilteredBlurServer(configuration, > shardServer, true); > > iface = ServerSecurityUtil.applySecurity(iface, serverSecurity, > true); > > > > > http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/9e9e8ed1/blur-core/src/main/java/org/apache/blur/thrift/ThriftServer.java > > ---------------------------------------------------------------------- > > diff --git > a/blur-core/src/main/java/org/apache/blur/thrift/ThriftServer.java > b/blur-core/src/main/java/org/apache/blur/thrift/ThriftServer.java > > index c7310ba..f1c3e94 100644 > > --- a/blur-core/src/main/java/org/apache/blur/thrift/ThriftServer.java > > +++ b/blur-core/src/main/java/org/apache/blur/thrift/ThriftServer.java > > @@ -22,10 +22,9 @@ import static > org.apache.blur.metrics.MetricsConstants.JVM; > > import static org.apache.blur.metrics.MetricsConstants.LOAD_AVERAGE; > > import static org.apache.blur.metrics.MetricsConstants.ORG_APACHE_BLUR; > > import static org.apache.blur.metrics.MetricsConstants.SYSTEM; > > -import static > org.apache.blur.utils.BlurConstants.BLUR_CONTROLLER_SERVER_SECURITY_CLASS; > > import static org.apache.blur.utils.BlurConstants.BLUR_HDFS_TRACE_PATH; > > import static org.apache.blur.utils.BlurConstants.BLUR_HOME; > > -import static > org.apache.blur.utils.BlurConstants.BLUR_SHARD_SERVER_SECURITY_CLASS; > > +import static > org.apache.blur.utils.BlurConstants.BLUR_SERVER_SECURITY_CLASS; > > import static > org.apache.blur.utils.BlurConstants.BLUR_ZOOKEEPER_TRACE_PATH; > > > > import java.io.BufferedReader; > > @@ -37,11 +36,15 @@ import java.lang.management.ManagementFactory; > > import java.lang.management.MemoryMXBean; > > import java.lang.management.MemoryUsage; > > import java.lang.management.OperatingSystemMXBean; > > -import java.lang.reflect.Constructor; > > import java.lang.reflect.Method; > > import java.net.InetAddress; > > import java.net.InetSocketAddress; > > import java.net.UnknownHostException; > > +import java.util.ArrayList; > > +import java.util.List; > > +import java.util.Map; > > +import java.util.Map.Entry; > > +import java.util.TreeMap; > > import java.util.UUID; > > import java.util.concurrent.ExecutorService; > > > > @@ -51,6 +54,7 @@ import org.apache.blur.log.Log; > > import org.apache.blur.log.LogFactory; > > import > org.apache.blur.manager.indexserver.BlurServerShutDown.BlurShutdown; > > import org.apache.blur.server.ServerSecurity; > > +import org.apache.blur.server.ServerSecurityFactory; > > import org.apache.blur.thirdparty.thrift_0_9_0.protocol.TBinaryProtocol; > > import > org.apache.blur.thirdparty.thrift_0_9_0.protocol.TCompactProtocol; > > import org.apache.blur.thirdparty.thrift_0_9_0.server.TServer; > > @@ -439,22 +443,36 @@ public class ThriftServer { > > } > > > > @SuppressWarnings("unchecked") > > - public static ServerSecurity getServerSecurity(BlurConfiguration > configuration, boolean shardServer) { > > - String className; > > - if (shardServer) { > > - className = configuration.get(BLUR_SHARD_SERVER_SECURITY_CLASS); > > - } else { > > - className = > configuration.get(BLUR_CONTROLLER_SERVER_SECURITY_CLASS); > > + public static List<ServerSecurity> > getServerSecurityList(BlurConfiguration configuration, > > + ServerSecurityFactory.ServerType type) { > > + Map<String, String> properties = configuration.getProperties(); > > + Map<String, String> classMap = new TreeMap<String, String>(); > > + for (Entry<String, String> e : properties.entrySet()) { > > + String property = e.getKey(); > > + String value = e.getValue(); > > + if (value == null || value.isEmpty()) { > > + continue; > > + } > > + if (property.startsWith(BLUR_SERVER_SECURITY_CLASS)) { > > + classMap.put(property, value); > > + } > > } > > - if (className == null) { > > + if (classMap.isEmpty()) { > > return null; > > } > > - try { > > - Class<? extends ServerSecurity> clazz = (Class<? extends > ServerSecurity>) Class.forName(className); > > - Constructor<? extends ServerSecurity> constructor = > clazz.getConstructor(new Class[] { BlurConfiguration.class }); > > - return constructor.newInstance(configuration); > > - } catch (Exception e) { > > - throw new RuntimeException(e); > > + List<ServerSecurity> result = new ArrayList<ServerSecurity>(); > > + for (Entry<String, String> entry : classMap.entrySet()) { > > + String className = entry.getValue(); > > + try { > > + LOG.info("Loading factory class [{0}]", className); > > + Class<? extends ServerSecurityFactory> clazz = (Class<? extends > ServerSecurityFactory>) Class > > + .forName(className); > > + ServerSecurityFactory serverSecurityFactory = > clazz.newInstance(); > > + result.add(serverSecurityFactory.getServerSecurity(type, > configuration)); > > I was concerned that this would feel awkward and I think it does. We > want the flexibility of chained filters which feels weird with a > factory pattern because we're really chaining factories here; but then > we want to vary by server type, so I dunno, maybe this is all that we > can do - just thinking aloud at this point... > > --tim > What do you think? Drop the factory pattern? Or no?
