incubator-blur-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aaron McCurry <amccu...@gmail.com>
Subject Re: git commit: Changing the server security feature to be driven by a factory pattern. It also supports a filter pattern for server security.
Date Tue, 03 Feb 2015 11:30:51 GMT
On Monday, February 2, 2015, Tim Williams <williamstw@gmail.com> wrote:

> On Mon, Feb 2, 2015 at 8:36 PM,  <amccurry@apache.org <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 <amccurry@gmail.com <javascript:;>>
> > Authored: Mon Feb 2 20:36:03 2015 -0500
> > Committer: Aaron McCurry <amccurry@gmail.com <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?

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message