[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/tinkerpop/pull/583


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[krlohnes]

Github user krlohnes commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/583#discussion_r109659946
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java 
---
@@ -384,9 +384,25 @@ public SerializerSettings() {}
  * used to load the implementation from the classpath. Defaults to 
{@link AllowAllAuthenticator} when
  * not specified.
  */
+public String authenticator = null;
+
+/**
+ * The fully qualified class name of the {@link Authenticator} 
implementation. This class name will be
+ * used to load the implementation from the classpath. Defaults to 
{@link AllowAllAuthenticator} when
+ * not specified.
+ * Deprecated in favor of {@link authenticator}.
--- End diff --

Fixed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[spmallette]

Github user spmallette commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/583#discussion_r109478243
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java 
---
@@ -384,9 +384,25 @@ public SerializerSettings() {}
  * used to load the implementation from the classpath. Defaults to 
{@link AllowAllAuthenticator} when
  * not specified.
  */
+public String authenticator = null;
+
+/**
+ * The fully qualified class name of the {@link Authenticator} 
implementation. This class name will be
+ * used to load the implementation from the classpath. Defaults to 
{@link AllowAllAuthenticator} when
+ * not specified.
+ * Deprecated in favor of {@link authenticator}.
--- End diff --

Note that our deprecation form in javadoc typically looks like this:

```java
@deprecated As of release 3.2.5, replaced by {@link authenticator}.
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[krlohnes]

Github user krlohnes commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/583#discussion_r109015202
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java 
---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
 public String className = AllowAllAuthenticator.class.getName();
 
 /**
+ * Enable audit logging of authenticated users and gremlin 
evaluation requests.
+ */
+public boolean enableAuditLog = false;
+
+/**
+ * The fully qualified class name of the {@link 
AuthenticationHandler} implementation.
+ * This class name will be used to load the implementation from 
the classpath.
+ * Defaults to null when not specified.
+ */
+public String handlerClassName = null;
--- End diff --

Fixed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[krlohnes]

Github user krlohnes commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/583#discussion_r109015184
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java 
---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
 public String className = AllowAllAuthenticator.class.getName();
 
 /**
+ * Enable audit logging of authenticated users and gremlin 
evaluation requests.
+ */
+public boolean enableAuditLog = false;
--- End diff --

Fixed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[krlohnes]

Github user krlohnes commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/583#discussion_r109015220
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/AuthenticationHandler.java
 ---
@@ -0,0 +1,52 @@
+/*
+ * 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.tinkerpop.gremlin.server.handler;
+
+import io.netty.channel.ChannelFutureListener;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.DefaultFullHttpResponse;
+import io.netty.handler.codec.http.FullHttpMessage;
+import io.netty.util.ReferenceCountUtil;
+import org.apache.tinkerpop.gremlin.server.auth.AuthenticationException;
+import org.apache.tinkerpop.gremlin.server.auth.Authenticator;
+
+import java.nio.charset.Charset;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.Map;
+
+import static io.netty.handler.codec.http.HttpResponseStatus.UNAUTHORIZED;
+import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1;
+import static 
org.apache.tinkerpop.gremlin.groovy.plugin.dsl.credential.CredentialGraphTokens.PROPERTY_PASSWORD;
+import static 
org.apache.tinkerpop.gremlin.groovy.plugin.dsl.credential.CredentialGraphTokens.PROPERTY_USERNAME;
+
+/**
+ * Provides an abstraction point to allow for http auth schemes beyond 
basic auth.
+ */
+import io.netty.channel.ChannelInboundHandlerAdapter;
+
+public abstract class AuthenticationHandler extends 
ChannelInboundHandlerAdapter {
--- End diff --

Fixed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[krlohnes]

Github user krlohnes commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/583#discussion_r108750598
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java 
---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
 public String className = AllowAllAuthenticator.class.getName();
 
 /**
+ * Enable audit logging of authenticated users and gremlin 
evaluation requests.
+ */
+public boolean enableAuditLog = false;
--- End diff --

Ah, I'll delete that.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[krlohnes]

Github user krlohnes commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/583#discussion_r108750337
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java 
---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
 public String className = AllowAllAuthenticator.class.getName();
 
 /**
+ * Enable audit logging of authenticated users and gremlin 
evaluation requests.
+ */
+public boolean enableAuditLog = false;
+
+/**
+ * The fully qualified class name of the {@link 
AuthenticationHandler} implementation.
+ * This class name will be used to load the implementation from 
the classpath.
+ * Defaults to null when not specified.
+ */
+public String handlerClassName = null;
--- End diff --

That sounds good to me.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[spmallette]

Github user spmallette commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/583#discussion_r108742664
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/AuthenticationHandler.java
 ---
@@ -0,0 +1,52 @@
+/*
+ * 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.tinkerpop.gremlin.server.handler;
+
+import io.netty.channel.ChannelFutureListener;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.DefaultFullHttpResponse;
+import io.netty.handler.codec.http.FullHttpMessage;
+import io.netty.util.ReferenceCountUtil;
+import org.apache.tinkerpop.gremlin.server.auth.AuthenticationException;
+import org.apache.tinkerpop.gremlin.server.auth.Authenticator;
+
+import java.nio.charset.Charset;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.Map;
+
+import static io.netty.handler.codec.http.HttpResponseStatus.UNAUTHORIZED;
+import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1;
+import static 
org.apache.tinkerpop.gremlin.groovy.plugin.dsl.credential.CredentialGraphTokens.PROPERTY_PASSWORD;
+import static 
org.apache.tinkerpop.gremlin.groovy.plugin.dsl.credential.CredentialGraphTokens.PROPERTY_USERNAME;
+
+/**
+ * Provides an abstraction point to allow for http auth schemes beyond 
basic auth.
+ */
+import io.netty.channel.ChannelInboundHandlerAdapter;
+
+public abstract class AuthenticationHandler extends 
ChannelInboundHandlerAdapter {
--- End diff --

We tend to prefix abstract` classes with "Abstract" as a convention. Also, 
please move the javadoc down below the `import` statement. You might also want 
to clean up the list of unused imports a bit.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[spmallette]

Github user spmallette commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/583#discussion_r108741558
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java 
---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
 public String className = AllowAllAuthenticator.class.getName();
 
 /**
+ * Enable audit logging of authenticated users and gremlin 
evaluation requests.
+ */
+public boolean enableAuditLog = false;
+
+/**
+ * The fully qualified class name of the {@link 
AuthenticationHandler} implementation.
+ * This class name will be used to load the implementation from 
the classpath.
+ * Defaults to null when not specified.
+ */
+public String handlerClassName = null;
--- End diff --

I tried to stay consistent in naming for classes in the config by going 
with `className` where the key was representative of an object in a `Map` or 
`List`, but now it kinda bites us as `authentication.className` is used for the 
`Authenticator` when it now really should probably refer to the handler since 
it is now configurable. 

Here's an idea:

1. Rename `handlerClassName` to `authenticationHandler`
2. Overload `className` with `authenticator` - if `authenticator` is 
present then ignore any value in `className`. Update all the yaml files to use 
`authenticator`. 
[Deprecate](http://tinkerpop.apache.org/docs/current/dev/developer/#_deprecation)
 `AuthenticationSettings.className`.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[spmallette]

Github user spmallette commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/583#discussion_r108735329
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java 
---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
 public String className = AllowAllAuthenticator.class.getName();
 
 /**
+ * Enable audit logging of authenticated users and gremlin 
evaluation requests.
+ */
+public boolean enableAuditLog = false;
--- End diff --

What is the purpose of this configuration option? It doesn't appear to be 
used in any way.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...

[krlohnes]

GitHub user krlohnes opened a pull request:

https://github.com/apache/tinkerpop/pull/583

TINKERPOP-1657 Provide abstraction to easily allow different HttpAuth 
schemes

Abstracting over the http authentication allows for easy extensibility
for users/implementors to provide their own classes for http auth beyond
basic auth. The general issue is that there is a fixed overhead to
hashing passwords securely. This change allows for implementing things
like HMAC token auth and plugging them in easily to the gremlin server.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/krlohnes/tinkerpop 
abstraction_for_different_http_auths

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/tinkerpop/pull/583.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #583


commit 1c7a4b18fd63f30c9da9a2b3a211c8f6fad1c596
Author: Keith Lohnes 
Date:   2017-03-20T17:37:40Z

Abstract over http auth for extensibility

Abstracting over the http authentication allows for easy extensibility
for users/implementors to provide their own classes for http auth beyond
basic auth. The general issue is that there is a fixed overhead to
hashing passwords securely. This change allows for implementing things
like HMAC token auth and plugging them in easily to the gremlin server.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---