ok, sounds fine to me On Tue, Feb 3, 2015 at 8:22 AM, Tim Williams <[email protected]> wrote:
> On Tue, Feb 3, 2015 at 6:30 AM, Aaron McCurry <[email protected]> wrote: > > 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? > > Well, as I said, I'm just thinking aloud at this point;) Maybe go > back to a regular interface with lifecycle methods? E.g. > > public interface ServerAccessFilter { > void init(ServerType type, BlurConfiguration conf); > void filter(...) > void close/exit/whatever > } > > That'd also give a chance to close any connections/handles down as well? > > --tim >
