mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r514036834
##########
File path:
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/ActivityRecordSet.java
##########
@@ -56,6 +58,19 @@
* If an error occurs while retrieving the records within this set.
*/
Collection<RecordType> asCollection() throws GuacamoleException;
+
+ /**
+ * Returns all records within this set as a list
+ *
+ * @return
+ * A list containing all records in this set.
+ *
+ * @throws GuacamoleException
+ * If an error occurs while retrieving the records within this set.
+ */
+ default List<RecordType> asList() throws GuacamoleException {
+ return new ArrayList<>(asCollection());
+ }
Review comment:
I'm not sure about the benefit of an `asList()` function outside our own
need for a convenient way to implement the now-deprecated functions. Perhaps
this shouldn't be part of the API? Will the benefit of this function survive
the eventual removal of the deprecated functions?
##########
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionService.java
##########
@@ -411,28 +411,16 @@ protected ConnectionRecord
getObjectInstance(ConnectionRecordModel model) {
ModeledConnection connection) throws GuacamoleException {
String identifier = connection.getIdentifier();
-
- // Retrieve history only if READ permission is granted
- if (hasObjectPermission(user, identifier, ObjectPermission.Type.READ))
{
Review comment:
Can you clarify why the explicit check for "READ" permission is no
longer needed?
##########
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/src/main/resources/org/apache/guacamole/auth/jdbc/user/UserRecordMapper.xml
##########
@@ -105,25 +105,32 @@
FROM guacamole_user_history
<!-- Search terms -->
- <foreach collection="terms" item="term"
- open="WHERE " separator=" AND ">
- (
-
- guacamole_user_history.user_id IN (
- SELECT user_id
- FROM guacamole_user
- JOIN guacamole_entity ON guacamole_user.entity_id =
guacamole_entity.entity_id
- WHERE
- POSITION(#{term.term,jdbcType=VARCHAR} IN
guacamole_entity.name) > 0
- AND guacamole_entity.type = 'USER'),
- )
-
- <if test="term.startDate != null and term.endDate != null">
- OR start_date BETWEEN #{term.startDate,jdbcType=TIMESTAMP}
AND #{term.endDate,jdbcType=TIMESTAMP}
- </if>
+ <where>
+
+ <if test="username != null">
+ guacamole_entity.name = #{username,jdbcType=VARCHAR}
Review comment:
I don't see where `guacamole_entity` is joined in here. Should this be
against `guacamole_user_history.username`?
##########
File path:
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleActivityRecordSet.java
##########
@@ -35,10 +36,30 @@
public class SimpleActivityRecordSet<RecordType extends ActivityRecord>
implements ActivityRecordSet<RecordType> {
+ private final List<RecordType> records;
+
+ /**
+ * Create a new SimpleActivityRecordSet that contains an empty set of
+ * records.
+ */
+ public SimpleActivityRecordSet() {
+ records = Collections.emptyList();
+ }
+
+ /**
+ * Create a new SimpleActivityRecordSet that contains the provided records.
+ *
+ * @param records
+ * The records that this SimpleActivityRecordSet should contain.
+ */
+ public SimpleActivityRecordSet(List<RecordType> records) {
+ this.records = records;
Review comment:
If the provided `List` will back the resulting `SimpleActivityRecordSet`
(such that changes to the `List` affect the content of the set), that should be
noted here.
##########
File path:
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleActivityRecordSet.java
##########
@@ -35,10 +36,30 @@
public class SimpleActivityRecordSet<RecordType extends ActivityRecord>
implements ActivityRecordSet<RecordType> {
+ private final List<RecordType> records;
Review comment:
Please document.
##########
File path:
guacamole/src/main/java/org/apache/guacamole/rest/user/UserResource.java
##########
@@ -110,18 +112,26 @@ public UserResource(@Assisted UserContext userContext,
* @throws GuacamoleException
* If an error occurs while retrieving the user history.
*/
- @GET
+ @SuppressWarnings("deprecation")
@Path("history")
- public List<APIActivityRecord> getUserHistory()
+ public UserHistoryResource getUserHistory()
throws GuacamoleException {
- // Retrieve the requested user's history
- List<APIActivityRecord> apiRecords = new
ArrayList<APIActivityRecord>();
- for (ActivityRecord record : user.getHistory())
- apiRecords.add(new APIActivityRecord(record));
-
- // Return the converted history
- return apiRecords;
+ try {
+ return new UserHistoryResource(user.getUserHistory());
+ }
+ catch (GuacamoleUnsupportedException e) {
+ try {
+ List<ActivityRecord> history = Collections.emptyList();
+ for (ActivityRecord record : user.getHistory()) {
+ history.add(record);
+ }
+ return new UserHistoryResource(new
SimpleActivityRecordSet<>(history));
+ }
+ catch (GuacamoleUnsupportedException ex) {
+ return new UserHistoryResource(new
SimpleActivityRecordSet<>());
+ }
+ }
Review comment:
Same here regarding possible logic cleanup, etc. (see
`ConnectionResource`)
##########
File path:
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Connection.java
##########
@@ -98,17 +99,55 @@
* of this Connection, including any active users. ConnectionRecords
* in this list will be sorted in descending order of end time (active
* connections are first), and then in descending order of start time
- * (newer connections are first).
+ * (newer connections are first). If connection history tracking is
+ * not implemented this method should throw GuacamoleUnsupportedException.
*
- * @return A list of ConnectionRecrods representing the usage history
- * of this Connection.
+ * @deprecated
+ * This function has been deprecated in favor of
+ * {@link getConnectionHistory}, which returns the connection history
+ * as an ActivityRecordSet that can be easily sorted and filtered.
+ * While the getHistory() method is provided for API compatibility,
+ * new implementations should avoid use of this method and, instead,
+ * implement the getConnectionHistory() method.
+ *
+ * @return
+ * A list of ConnectionRecrods representing the usage history of this
+ * Connection.
*
- * @throws GuacamoleException If an error occurs while reading the history
- * of this connection, or if permission is
- * denied.
+ * @throws GuacamoleException
+ * If history tracking is not implemented, if an error occurs while
+ * reading the history of this connection, or if permission is
+ * denied.
*/
- public List<? extends ConnectionRecord> getHistory() throws
GuacamoleException;
+ @Deprecated
+ default List<? extends ConnectionRecord> getHistory()
+ throws GuacamoleException {
+ return getConnectionHistory().asList();
+ }
+ /**
+ * Returns an ActivityRecordSet containing ConnectionRecords that
+ * represent the usage history of this Connection, including any active
+ * users. ConnectionRecords in this list will be sorted in descending order
+ * of end time (active connections are first), and then in descending order
+ * of start time (newer connections are first). If connection history
+ * tracking has not been implemented this function should throw
+ * GuacamoleUnsupportExpcetion.
Review comment:
Point of clarification: `GuacamoleUnsupportedException` would be thrown
if history tracking has not been implemented _**or**_ if it has only been
implemented using the older, deprecated `getHistory()` function. Callers
intending to support extensions using older versions of the API should consider
additionally invoking `getHistory()` if `GuacamoleUnsupportedException` is
thrown.
##########
File path:
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/DelegatingUser.java
##########
@@ -93,11 +93,18 @@ public Date getLastActive() {
return user.getLastActive();
}
+ @Deprecated
@Override
public List<? extends ActivityRecord> getHistory()
Review comment:
I've seen this applied a few times to implementations of the
now-deprecated functions. Is the `@Deprecated` annotation not inherited from
the interface that deprecated the overridden function?
##########
File path: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/User.java
##########
@@ -93,17 +94,49 @@
* of this user, including any active sessions. ActivityRecords
* in this list will be sorted in descending order of end time (active
* sessions are first), and then in descending order of start time
- * (newer sessions are first).
+ * (newer sessions are first). If user login history is not implemented
+ * this method should throw GuacamoleUnsupportedException.
Review comment:
Same here - I think we should specifically note that
`GuacamoleUnsupportedException` may indicate that history is supported via
`getHistory()`.
##########
File path:
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Connection.java
##########
@@ -98,17 +99,55 @@
* of this Connection, including any active users. ConnectionRecords
* in this list will be sorted in descending order of end time (active
* connections are first), and then in descending order of start time
- * (newer connections are first).
+ * (newer connections are first). If connection history tracking is
+ * not implemented this method should throw GuacamoleUnsupportedException.
*
- * @return A list of ConnectionRecrods representing the usage history
- * of this Connection.
+ * @deprecated
+ * This function has been deprecated in favor of
+ * {@link getConnectionHistory}, which returns the connection history
+ * as an ActivityRecordSet that can be easily sorted and filtered.
+ * While the getHistory() method is provided for API compatibility,
+ * new implementations should avoid use of this method and, instead,
+ * implement the getConnectionHistory() method.
+ *
+ * @return
+ * A list of ConnectionRecrods representing the usage history of this
+ * Connection.
*
- * @throws GuacamoleException If an error occurs while reading the history
- * of this connection, or if permission is
- * denied.
+ * @throws GuacamoleException
+ * If history tracking is not implemented, if an error occurs while
+ * reading the history of this connection, or if permission is
+ * denied.
*/
- public List<? extends ConnectionRecord> getHistory() throws
GuacamoleException;
+ @Deprecated
+ default List<? extends ConnectionRecord> getHistory()
+ throws GuacamoleException {
+ return getConnectionHistory().asList();
+ }
+ /**
+ * Returns an ActivityRecordSet containing ConnectionRecords that
+ * represent the usage history of this Connection, including any active
+ * users. ConnectionRecords in this list will be sorted in descending order
+ * of end time (active connections are first), and then in descending order
+ * of start time (newer connections are first). If connection history
+ * tracking has not been implemented this function should throw
+ * GuacamoleUnsupportExpcetion.
Review comment:
`GuacamoleUnsupportException`*
##########
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/src/main/resources/org/apache/guacamole/auth/jdbc/connection/ConnectionRecordMapper.xml
##########
@@ -108,28 +108,35 @@
LEFT JOIN guacamole_user ON guacamole_connection_history.user_id
= guacamole_user.user_id
<!-- Search terms -->
- <foreach collection="terms" item="term"
- open="WHERE " separator=" AND ">
- (
-
- guacamole_connection_history.user_id IN (
- SELECT user_id
- FROM guacamole_user
- WHERE POSITION(#{term.term,jdbcType=VARCHAR} IN username)
> 0
- )
+ <where>
+
+ <if test="identifier != null">
+ guacamole_connection_history.connection_id =
#{identifier,jdbcType=VARCHAR}
+ </if>
+
+ <foreach collection="terms" item="term" separator=" AND ">
Review comment:
Will the `AND` be automatically inserted here between
`guacamole_connection_history.connection_id = ...` and `(term1) AND (term2) AND
(term3) AND ...`?
##########
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/UserService.java
##########
@@ -581,13 +581,9 @@ protected ActivityRecord
getObjectInstance(ActivityRecordModel model) {
ModeledUser user) throws GuacamoleException {
String username = user.getIdentifier();
-
- // Retrieve history only if READ permission is granted
- if (hasObjectPermission(authenticatedUser, username,
ObjectPermission.Type.READ))
- return getObjectInstances(userRecordMapper.select(username));
Review comment:
Same here - why remove the check?
##########
File path:
guacamole/src/main/java/org/apache/guacamole/rest/connection/ConnectionResource.java
##########
@@ -150,18 +153,26 @@ public ConnectionResource(@Assisted UserContext
userContext,
* @throws GuacamoleException
* If an error occurs while retrieving the connection history.
*/
- @GET
+ @SuppressWarnings("deprecation")
@Path("history")
- public List<APIConnectionRecord> getConnectionHistory()
+ public ConnectionHistoryResource getConnectionHistory()
throws GuacamoleException {
- // Retrieve the requested connection's history
- List<APIConnectionRecord> apiRecords = new
ArrayList<APIConnectionRecord>();
- for (ConnectionRecord record : connection.getHistory())
- apiRecords.add(new APIConnectionRecord(record));
-
- // Return the converted history
- return apiRecords;
+ try {
+ return new
ConnectionHistoryResource(connection.getConnectionHistory());
+ }
+ catch (GuacamoleUnsupportedException e) {
+ try {
+ List<ConnectionRecord> history = Collections.emptyList();
+ for (ConnectionRecord record : connection.getHistory()) {
+ history.add(record);
+ }
+ return new ConnectionHistoryResource(new
SimpleActivityRecordSet<>(history));
+ }
+ catch (GuacamoleUnsupportedException ex) {
+ return new ConnectionHistoryResource(new
SimpleActivityRecordSet<>());
+ }
+ }
Review comment:
A couple of points here:
1. The logic here is involved enough that I think it deserves some comments.
2. It's worth logging these exceptions with context at the debug level.
3. Because each `try` would `return` from the function entirely, you can
avoid nesting the `try`/`catch` deeper and deeper with something like:
```java
// Try to do it
try {
return doSomething();
}
catch (GuacamoleUnsupportedException e) {
logger.debug("Useful context.", e);
}
// Well that didn't work
try {
return doSomethingElse();
}
catch (GuacamoleUnsupportedException e) {
logger.debug("Useful context.", e);
}
// Darn
return doLoseAllHope();
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
