On Mon, Feb 2, 2015 at 8:36 PM, <[email protected]> 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]>
> Authored: Mon Feb 2 20:36:03 2015 -0500
> Committer: Aaron McCurry <[email protected]>
> 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
