GUACAMOLE-1130: Correct logic for attribute search and retrieval, and apply attribute filters to User and Connection searches.

extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ObjectQueryService.java

@@ -198,9 +198,10 @@ public class ObjectQueryService {
      *     The current level of referral depth for this search, used for
      *     limiting the maximum depth to which referrals can go.
      * 
-     * @param relevantAttributes
+     * @param attributes
-     *     The attribute(s) relevant to return for this search, or null if all
+     *     A collection of the names of attributes that should be retrieved
-     *     available attributes should be returned.
+     *     from LDAP entries returned by the search, or null if all available
+     *     attributes should be returned.
      *
      * @return
      *     A list of all results accessible to the user currently bound under
@@ -213,7 +214,7 @@ public class ObjectQueryService {
      */
     public List<Entry> search(LdapNetworkConnection ldapConnection,
             Dn baseDN, ExprNode query, int searchHop,
-            Collection<String> relevantAttributes) throws GuacamoleException {
+            Collection<String> attributes) throws GuacamoleException {
 
         // Refuse to follow referrals if limit has been reached
         int maxHops = confService.getMaxReferralHops();
@@ -231,8 +232,8 @@ public class ObjectQueryService {
         // Search within subtree of given base DN
         SearchRequest request = ldapService.getSearchRequest(baseDN, query);
         
-        if (relevantAttributes != null)
+        if (attributes != null)
-            request.addAttributes(relevantAttributes.toArray(new String[0]));
+            request.addAttributes(attributes.toArray(new String[0]));
 
         // Produce list of all entries in the search result, automatically
         // following referrals if configured to do so
@@ -259,7 +260,7 @@ public class ObjectQueryService {
                             try (LdapNetworkConnection referralConnection = ldapService.bindAs(url, ldapConnection)) {
                                 if (referralConnection != null) {
                                     logger.debug("Following referral to \"{}\"...", url);
-                                    entries.addAll(search(referralConnection, baseDN, query, searchHop + 1, relevantAttributes));
+                                    entries.addAll(search(referralConnection, baseDN, query, searchHop + 1, attributes));
                                 }
                                 else
                                     logger.debug("Could not bind with LDAP "
@@ -314,15 +315,20 @@ public class ObjectQueryService {
      *     The LDAP filter to apply to reduce the results of the query in
      *     addition to testing the values of the given attributes.
      *
-     * @param attributes
+     * @param filterAttributes
      *     A collection of all attributes to test for equivalence to the given
      *     value, in order of decreasing priority.
      *
-     * @param attributeValue
+     * @param filterValue
      *     The value that should be searched search for within the attributes
      *     of objects within the LDAP directory. If null, the search will test
      *     only for the presence of at least one of the given attributes on
      *     each object, regardless of the value of those attributes.
+     * 
+     * @param attributes
+     *     A collection of the names of attributes that should be retrieved
+     *     from LDAP entries returned by the search, or null if all available
+     *     attributes should be returned.
      *
      * @return
      *     A list of all results accessible to the user currently bound under
@@ -334,9 +340,10 @@ public class ObjectQueryService {
      *     guacamole.properties.
      */
     public List<Entry> search(LdapNetworkConnection ldapConnection, Dn baseDN,
-            ExprNode filter, Collection<String> attributes, String attributeValue)
+            ExprNode filter, Collection<String> filterAttributes, String filterValue,
+            Collection<String> attributes)
             throws GuacamoleException {
-        ExprNode query = generateQuery(filter, attributes, attributeValue);
+        ExprNode query = generateQuery(filter, filterAttributes, filterValue);
         return search(ldapConnection, baseDN, query, 0, attributes);
     }
 

extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java

@@ -20,7 +20,10 @@
 package org.apache.guacamole.auth.ldap.connection;
 
 import com.google.inject.Inject;
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import org.apache.directory.api.ldap.model.entry.Attribute;
@@ -77,6 +80,48 @@ public class ConnectionService {
      */
     @Inject
     private UserGroupService userGroupService;
+    
+    /**
+     * The objectClass that is present on any Guacamole connections stored
+     * in LDAP.
+     */
+    public static final String CONNECTION_LDAP_OBJECT_CLASS = "guacConfigGroup";
+    
+    /**
+     * The attribute name that uniquely identifies a Guacamole connection object
+     * in LDAP.
+     */
+    public static final String LDAP_ATTRIBUTE_NAME_ID = "cn";
+    
+    /**
+     * The LDAP attribute name where the Guacamole connection protocol is stored.
+     */
+    public static final String LDAP_ATTRIBUTE_NAME_PROTOCOL = "guacConfigProtocol";
+    
+    /**
+     * The LDAP attribute name that contains any connection parameters.
+     */
+    public static final String LDAP_ATTRIBUTE_NAME_PARAMETER = "guacConfigParameter";
+    
+    /**
+     * The LDAP attribute name that provides group-based access control for
+     * Guacamole connection objects.
+     */
+    public static final String LDAP_ATTRIBUTE_NAME_GROUPS = "seeAlso";
+    
+    /**
+     * A list of all attribute names that could be associated with a Guacamole
+     * connection object in LDAP.
+     */
+    public static final Collection<String> GUAC_CONFIG_LDAP_ATTRIBUTES =
+            Collections.unmodifiableSet(new HashSet<String>(Arrays.asList(
+                    LDAP_ATTRIBUTE_NAME_ID,
+                    LDAP_ATTRIBUTE_NAME_PROTOCOL,
+                    LDAP_ATTRIBUTE_NAME_PARAMETER,
+                    LDAP_ATTRIBUTE_NAME_GROUPS
+            )));
+            
+            
 
     /**
      * Returns all Guacamole connections accessible to the user currently bound
@@ -126,16 +171,17 @@ public class ConnectionService {
             // and possibly any groups the user is a member of that are
             // referred to in the seeAlso attribute of the guacConfigGroup.
             List<Entry> results = queryService.search(ldapConnection,
-                    configurationBaseDN, connectionSearchFilter, 0, null);
+                    configurationBaseDN, connectionSearchFilter, 0, GUAC_CONFIG_LDAP_ATTRIBUTES);
 
             // Return a map of all readable connections
             return queryService.asMap(results, (entry) -> {
 
                 // Get common name (CN)
-                Attribute cn = entry.get("cn");
+                Attribute cn = entry.get(LDAP_ATTRIBUTE_NAME_ID);
                 
                 if (cn == null) {
-                    logger.warn("guacConfigGroup is missing a cn.");
+                    logger.warn("{} is missing a {}.",
+                            CONNECTION_LDAP_OBJECT_CLASS, LDAP_ATTRIBUTE_NAME_ID);
                     return null;
                 }
                 
@@ -145,18 +191,19 @@ public class ConnectionService {
                     cnName = cn.getString();
                 }
                 catch (LdapInvalidAttributeValueException e) {
-                    logger.error("Invalid value for CN attribute: {}",
+                    logger.error("Invalid value for {} attribute: {}",
-                            e.getMessage());
+                            LDAP_ATTRIBUTE_NAME_ID, e.getMessage());
                     logger.debug("LDAP exception while getting CN attribute.", e);
                     return null;
                 }
 
                 // Get associated protocol
-                Attribute protocol = entry.get("guacConfigProtocol");
+                Attribute protocol = entry.get(LDAP_ATTRIBUTE_NAME_PROTOCOL);
                 if (protocol == null) {
-                    logger.warn("guacConfigGroup \"{}\" is missing the "
+                    logger.warn("{} \"{}\" is missing the "
-                              + "required \"guacConfigProtocol\" attribute.",
+                              + "required \"{}\" attribute.",
-                            cnName);
+                            CONNECTION_LDAP_OBJECT_CLASS,
+                            cnName, LDAP_ATTRIBUTE_NAME_PROTOCOL);
                     return null;
                 }
 
@@ -173,7 +220,7 @@ public class ConnectionService {
                 }
 
                 // Get parameters, if any
-                Attribute parameterAttribute = entry.get("guacConfigParameter");
+                Attribute parameterAttribute = entry.get(LDAP_ATTRIBUTE_NAME_PARAMETER);
                 if (parameterAttribute != null) {
 
                     // For each parameter
@@ -256,7 +303,7 @@ public class ConnectionService {
         AndNode searchFilter = new AndNode();
 
         // Add the prefix to the search filter, prefix filter searches for guacConfigGroups with the userDN as the member attribute value
-        searchFilter.addNode(new EqualityNode("objectClass","guacConfigGroup"));
+        searchFilter.addNode(new EqualityNode("objectClass", CONNECTION_LDAP_OBJECT_CLASS));
         
         // Apply group filters
         OrNode groupFilter = new OrNode();
@@ -268,7 +315,7 @@ public class ConnectionService {
         List<Entry> userGroups = userGroupService.getParentUserGroupEntries(ldapConnection, userDN);
         if (!userGroups.isEmpty()) {
             userGroups.forEach(entry ->
-                groupFilter.addNode(new EqualityNode("seeAlso",entry.getDn().toString()))
+                groupFilter.addNode(new EqualityNode(LDAP_ATTRIBUTE_NAME_GROUPS,entry.getDn().toString()))
             );
         }
 

extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java

@@ -123,6 +123,11 @@ public class UserGroupService {
         if (groupBaseDN == null)
             return Collections.emptyMap();
 
+        // Gather all attributes relevant for a group
+        String memberAttribute = confService.getMemberAttribute();
+        Collection<String> groupAttributes = new HashSet<>(confService.getGroupNameAttributes());
+        groupAttributes.add(memberAttribute);
+
         // Retrieve all visible user groups which are not guacConfigGroups
         Collection<String> attributes = confService.getGroupNameAttributes();
         List<Entry> results = queryService.search(
@@ -130,7 +135,8 @@ public class UserGroupService {
             groupBaseDN,
             getGroupSearchFilter(),
             attributes,
-            null
+            null,
+            groupAttributes
         );
 
         // Convert retrieved user groups to map of identifier to Guacamole
@@ -186,6 +192,7 @@ public class UserGroupService {
         // memberAttribute specified in properties could contain DN or username 
         MemberAttributeType memberAttributeType = confService.getMemberAttributeType();
         String userIDorDN = userDN.toString();
+        Collection<String> userAttributes = confService.getUsernameAttributes();
         if (memberAttributeType == MemberAttributeType.UID) {
             // Retrieve user objects with userDN
             List<Entry> userEntries = queryService.search(
@@ -193,7 +200,7 @@ public class UserGroupService {
                 userDN,
                 confService.getUserSearchFilter(),
                 0,
-                null);
+                userAttributes);
             // ... there can surely only be one
             if (userEntries.size() != 1)
                 logger.warn("user DN \"{}\" does not return unique value "
@@ -201,7 +208,6 @@ public class UserGroupService {
             else {
                 // determine unique identifier for user
                 Entry userEntry = userEntries.get(0);
-                Collection<String> userAttributes = confService.getUsernameAttributes();
                 try {
                     userIDorDN = queryService.getIdentifier(userEntry,
                                          userAttributes);
@@ -216,8 +222,9 @@ public class UserGroupService {
         }
 
         // Gather all attributes relevant for a group
-        List<String> groupAttributes = confService.getGroupNameAttributes();
+        String memberAttribute = confService.getMemberAttribute();
-        groupAttributes.add(confService.getMemberAttribute());
+        Collection<String> groupAttributes = new HashSet<>(confService.getGroupNameAttributes());
+        groupAttributes.add(memberAttribute);
 
         // Get all groups the user is a member of starting at the groupBaseDN,
         // excluding guacConfigGroups
@@ -225,8 +232,9 @@ public class UserGroupService {
             ldapConnection,
             groupBaseDN,
             getGroupSearchFilter(),
-            groupAttributes,
+            Collections.singleton(memberAttribute),
-            userIDorDN
+            userIDorDN,
+            groupAttributes
         );
 
     }

extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java

@@ -22,6 +22,8 @@ package org.apache.guacamole.auth.ldap.user;
 import com.google.inject.Inject;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import org.apache.directory.api.ldap.model.entry.Entry;
@@ -83,12 +85,15 @@ public class UserService {
             throws GuacamoleException {
 
         // Retrieve all visible user objects
-        Collection<String> attributes = confService.getUsernameAttributes();
+        Collection<String> usernameAttrs = confService.getUsernameAttributes();
+        Collection<String> attributes = new HashSet<>(usernameAttrs);
+        attributes.addAll(confService.getAttributes());
         List<Entry> results = queryService.search(ldapConnection,
                 confService.getUserBaseDN(),
                 confService.getUserSearchFilter(),
-                attributes,
+                usernameAttrs,
-                null);
+                null,
+                attributes);
 
         // Convert retrieved users to map of identifier to Guacamole user object
         return queryService.asMap(results, entry -> {
@@ -142,7 +147,8 @@ public class UserService {
                 confService.getUserBaseDN(),
                 confService.getUserSearchFilter(),
                 confService.getUsernameAttributes(),
-                username);
+                username,
+                Collections.singletonList("dn"));
 
         // Build list of all DNs for retrieved users
         List<Dn> userDNs = new ArrayList<>(results.size());