[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #730: GUACAMOLE-1616: Track external connection history in JDBC extensions

[GitBox <gi...@apache.org>]

mike-jumper commented on code in PR #730:
URL: https://github.com/apache/guacamole-client/pull/730#discussion_r884008284


##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/HistoryTrackingUserContext.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.guacamole.auth.jdbc;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.auth.jdbc.connection.ConnectionRecordMapper;
+import org.apache.guacamole.net.auth.Connection;
+import org.apache.guacamole.net.auth.DelegatingUserContext;
+import org.apache.guacamole.net.auth.Directory;
+import org.apache.guacamole.net.auth.UserContext;
+
+/**
+ * DelegatingUserContext implementation which writes connection history records
+ * when connections are established and closed.
+ */
+public class HistoryTrackingUserContext extends DelegatingUserContext {
+
+    /**
+     * The remote host that the user associated with the user context
+     * connected from.
+     */
+    private final String remoteHost;
+
+    /**
+     * The connection record mapper to use when writing history entries for
+     * established connections.
+     */
+    private final ConnectionRecordMapper connectionRecordMapper;
+
+    /**
+     * Creates a new HistoryTrackingUserContext which wraps the given
+     * UserContext, allowing for tracking of connection history external to
+     * this authentication provider.
+     *
+     * @param userContext
+     *     The UserContext to wrap.
+     *
+     * @param string
+     *
+     * @param connectionRecordMapper
+     *     The mapper to use when writing connection history entries to the DB.
+     */
+    public HistoryTrackingUserContext(UserContext userContext, String remoteHost, ConnectionRecordMapper connectionRecordMapper) {
+        super(userContext);
+
+        this.remoteHost = remoteHost;
+        this.connectionRecordMapper = connectionRecordMapper;
+    }
+
+    @Override
+    public Directory<Connection> getConnectionDirectory() throws GuacamoleException {
+        return new HistoryTrackingConnectionDirectory(
+                super.getConnectionDirectory(), this.self(),

Review Comment:
   Why `super` for `getConnectionDirectory()` but `this` for `self()`? Why not just `getConnectionDirectory()` and `self()`?



##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/HistoryTrackingConnectionDirectory.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.guacamole.auth.jdbc;
+
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.auth.jdbc.connection.ConnectionRecordMapper;
+import org.apache.guacamole.net.auth.Connection;
+import org.apache.guacamole.net.auth.DelegatingDirectory;
+import org.apache.guacamole.net.auth.Directory;
+import org.apache.guacamole.net.auth.User;
+
+/**
+ * A connection directory that returns HistoryTrackingConnection-wrapped connections
+ * when queried.
+ */
+public class HistoryTrackingConnectionDirectory extends DelegatingDirectory<Connection> {

Review Comment:
   This should instead extend `DecoratingDirectory`.
   
   The `update()` function of `Directory` needs to receive the same object returned by `get()` of that `Directory` implementation. If a decorated `Directory` returns a different object from `get()`, it needs to unwrap that object and return the original within `update()`. The `DecoratingDirectory` convenience object does that for you.



##########
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/UserContext.java:
##########
@@ -103,7 +103,7 @@ public interface UserContext {
      * connections and their configurations, but only as allowed by the
      * permissions given to the user.
      *
-     * @return A Directory whose operations are bound by the permissions of 
+     * @return A Directory whose operations are bound by the permissions of

Review Comment:
   Why is this file modified?



##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/conf/PostgreSQLGuacamoleProperties.java:
##########
@@ -202,79 +202,92 @@ private PostgreSQLGuacamoleProperties() {}
         public String getName() { return "postgresql-default-max-group-connections-per-user"; }
 
     };
-    
+
     /**
      * The SSL mode that should be used by the JDBC driver when making
      * connections to the remote server.  By default SSL will be attempted but
      * plain-text will be allowed if SSL fails.
      */
     public static final EnumGuacamoleProperty<PostgreSQLSSLMode> POSTGRESQL_SSL_MODE =
             new EnumGuacamoleProperty<PostgreSQLSSLMode>(PostgreSQLSSLMode.class) {
-        
+
         @Override
         public String getName() { return "postgresql-ssl-mode"; }
-        
+
     };
-    
+
     /**
      * The client SSL certificate file used by the JDBC driver to make the
      * SSL connection.
      */
     public static final FileGuacamoleProperty POSTGRESQL_SSL_CERT_FILE =
             new FileGuacamoleProperty() {
-             
+
         @Override
         public String getName() { return "postgresql-ssl-cert-file"; }
-                
+
     };
-    
+
     /**
      * The client SSL private key file used by the JDBC driver to make the
      * SSL connection.
      */
     public static final FileGuacamoleProperty POSTGRESQL_SSL_KEY_FILE =
             new FileGuacamoleProperty() {
-    
+
         @Override
         public String getName() { return "postgresql-ssl-key-file"; }
-        
+
     };
-    
+
     /**
      * The client SSL root certificate file used by the JDBC driver to validate
      * certificates when making the SSL connection.
      */
     public static final FileGuacamoleProperty POSTGRESQL_SSL_ROOT_CERT_FILE =
             new FileGuacamoleProperty() {
-        
+
         @Override
         public String getName() { return "postgresql-ssl-root-cert-file"; }
-        
+
     };
-    
+
     /**
      * The password of the SSL private key used by the JDBC driver to make
      * the SSL connection to the PostgreSQL server.
      */
     public static final StringGuacamoleProperty POSTGRESQL_SSL_KEY_PASSWORD =
             new StringGuacamoleProperty() {
-        
+
         @Override
         public String getName() { return "postgresql-ssl-key-password"; }
-        
+
     };
-    
+
     /**
      * Whether or not to automatically create accounts in the PostgreSQL
      * database for users who successfully authenticate through another
      * extension. By default users will not be automatically created.
      */
     public static final BooleanGuacamoleProperty POSTGRESQL_AUTO_CREATE_ACCOUNTS =
             new BooleanGuacamoleProperty() {
-                
+
         @Override
         public String getName() { return "postgresql-auto-create-accounts"; }
-                
+
     };
-    
+
+    /**
+     * Whether or not to track connection history for connections that do not originate
+     * from within the Postgres database. By default, external connection history will be
+     * tracked.
+     */
+    public static final BooleanGuacamoleProperty POSTGRESQL_TRACK_EXTERNAL_CONNECTION_HISTORY =
+            new BooleanGuacamoleProperty() {
+
+        @Override
+        public String getName() { return "postgres-track-external-connection-history"; }

Review Comment:
   This should be `postgresql-*` for consistency.



##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java:
##########
@@ -505,4 +506,13 @@ public boolean isFailoverOnly() {
         return getModel().isFailoverOnly();
     }
 
+    /**
+     * Returns the JDBC environment associated with this connection.
+     *
+     * @return the JDBC environment associated with this connection.
+     */
+    public JDBCEnvironment gEnvironment() {
+        return this.environment;
+    }

Review Comment:
   Is this called anywhere? If not, this should be removed. If it is, then:
   
   1. Please format the JavaDoc to match the formatting used elsewhere (new line after `@return`, sentence case for the info that follows, indented).
   2. This should be `getEnvironment()`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org