[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #550: GUACAMOLE-1130: GUACAMOLE-1130: Only retrieve LDAP attributes that are strictly necessary

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

mike-jumper commented on a change in pull request #550:
URL: https://github.com/apache/guacamole-client/pull/550#discussion_r460610448



##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java
##########
@@ -208,13 +208,21 @@ private ExprNode getGroupSearchFilter() throws GuacamoleException {
             }
         }
 
+        // Gather all attributes relevant for a group
+        ArrayList<String> groupAttributes = new ArrayList<String>();

Review comment:
       1. The size of the required backing array can be anticipated here. This will work as-is, but unnecessarily relies on automatic array resizing or sufficiently-large defaults.
   2. Unless code consuming this relies on the object being an `ArrayList`, the type declared for the variable should be kept minimal, presumably `List<String>` or `Collection<String>`.

##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ObjectQueryService.java
##########
@@ -197,6 +197,10 @@ public ExprNode generateQuery(ExprNode filter,
      * @param searchHop
      *     The current level of referral depth for this search, used for
      *     limiting the maximum depth to which referrals can go.
+     * 
+     * @param relevantAttributes
+     *     The attribute(s) relevant to return for this search,
+	 *     if all available should be returned pass null as value.

Review comment:
       1. Please correct the alignment here. It looks like a tab character may have snuck in.
   2. Documentation should describe the nature/meaning of things, rather than commanding the developer to do a specific thing under specific circumstances. The first half of the documentation for this parameter looks good, but the latter half switches over from descriptive to imperative. I think we should maintain the established descriptive phrasing (for example: "or null if all attributes should be returned").

##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java
##########
@@ -208,13 +208,21 @@ private ExprNode getGroupSearchFilter() throws GuacamoleException {
             }
         }
 
+        // Gather all attributes relevant for a group
+        ArrayList<String> groupAttributes = new ArrayList<String>();
+        groupAttributes.add(confService.getMemberAttribute());
+        confService.getGroupNameAttributes().forEach(
+                       attribute -> groupAttributes.add(attribute)
+                       );

Review comment:
       Why not `addAll()`?
   
   And what if one of the attribute names within `confService.getGroupNameAttributes()` is equal to `confService.getMemberAttribute()`? Will the duplicate attribute cause any problems for the search?




----------------------------------------------------------------
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:
users@infra.apache.org