LdapUser should be an attribute wrapper

Author: Vladimir Kotal

plugins/src/opengrok/auth/entity/LdapUser.java

@@ -26,94 +26,53 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
-import java.util.TreeSet;
 
 /**
+ * LDAP user represented as a set of attributes
  *
  * @author Krystof Tulinger
  */
 public class LdapUser implements Serializable {
 
-    private String mail;
-    private String uid;
-    private Set<String> ou;
-    private final Map<String, Set<String>> attrs = new HashMap<>();
+    private final Map<String, Set<String>> attributes;
 
     // Use default serial ID value. If the serialized form of the object
     // changes, feel free to start from 1L.
-    private static final long serialVersionUID = -8207597677599370334L;
+    private static final long serialVersionUID = 1161431688782569843L;
 
-    public LdapUser(String mail, String uid, Set<String> ou) {
-        this.mail = mail;
-        this.uid = uid;
-        this.ou = ou == null ? new TreeSet<>() : ou;
+    public LdapUser() {
+        this(null);
     }
 
-    public String getMail() {
-        return mail;
-    }
-
-    public void setMail(String mail) {
-        this.mail = mail;
-    }
-
-    public String getUid() {
-        return uid;
-    }
-
-    public void setUid(String uid) {
-        this.uid = uid;
-    }
-
-    public Set<String> getOu() {
-        return ou;
-    }
-
-    public void setOu(Set<String> ou) {
-        this.ou = ou == null ? new TreeSet<>() : ou;
-    }
-
-    public boolean hasOu(String name) {
-        return this.ou.contains(name);
+    public LdapUser(Map<String, Set<String>> attrs) {
+        if (attrs == null) {
+            this.attributes = new HashMap<>();
+        } else {
+            this.attributes = attrs;
+        }
     }
 
     /**
-     * Get custom user property.
+     * set attribute value
      *
      * @param key the key
-     * @return the the value associated with the key
-     */
-    public Object getAttribute(String key) {
-        return attrs.get(key);
-    }
-
-    /**
-     * Set custom user property.
-     *
-     * @param key the key
-     * @param value the value
+     * @param value set of values
      * @return the value previously associated with the key
      */
     public Object setAttribute(String key, Set<String> value) {
-        return attrs.put(key, value);
+        return attributes.put(key, value);
     }
 
-    /**
-     * Remote custom user property.
-     *
-     * @param key the key
-     * @return the value previously associated with the key
-     */
-    public Object removeAttribute(String key) {
-        return attrs.remove(key);
+    public Set<String> getAttribute(String key) {
+        return attributes.get(key);
     }
 
     public Map<String, Set<String>> getAttributes() {
-        return this.attrs;
+        return this.attributes;
     }
 
     @Override
     public String toString() {
-        return "LdapUser{" + "mail=" + mail + ", uid=" + uid + ", ou=" + ou + '}';
+        return "LdapUser{attributes=" + attributes + '}';
     }
 }

plugins/src/opengrok/auth/plugin/LdapAttrPlugin.java

@@ -42,7 +42,7 @@
  */
 public class LdapAttrPlugin extends AbstractLdapPlugin {
 
-    protected static final String ATTR_PARAM = "attribute";
+    protected static final String ATTR_PARAM = "attribute"; // LDAP attribute name to check
     protected static final String FILE_PARAM = "file";
 
     private static final String SESSION_ALLOWED_PREFIX = "opengrok-attr-plugin-allowed";
@@ -87,33 +87,31 @@ public void fillSession(HttpServletRequest req, User user) {
         Boolean sessionAllowed = false;
         LdapUser ldapUser;
         Map<String, Set<String>> records;
-        Set<String> attributes;
+        Set<String> attributeValues;
 
         updateSession(req, sessionAllowed);
 
         if ((ldapUser = (LdapUser) req.getSession().getAttribute(LdapUserPlugin.SESSION_ATTR)) == null) {
+            // TODO log
             return;
         }
 
-        if (ldapAttr.equals("mail")) {
-            sessionAllowed = whitelist.contains(ldapUser.getMail());
-        } else if (ldapAttr.equals("uid")) {
-            sessionAllowed = whitelist.contains(ldapUser.getUid());
-        } else if (ldapAttr.equals("ou")) {
-            sessionAllowed = ldapUser.getOu().stream().anyMatch((t) -> whitelist.contains(t));
-        } else if ((attributes = (Set<String>) ldapUser.getAttribute(ldapAttr)) != null) {
-            sessionAllowed = attributes.stream().anyMatch((t) -> whitelist.contains(t));
+        // Check attributes cached in LDAP user object first, then query LDAP server
+        // (and if found, cache the result in the LDAP user object).
+        attributeValues = ldapUser.getAttribute(ldapAttr);
+        if (attributeValues != null) {
+            sessionAllowed = attributeValues.stream().anyMatch((t) -> whitelist.contains(t));
         } else {
             if ((records = getLdapProvider().lookupLdapContent(user, new String[]{ldapAttr})) == null) {
                 return;
             }
 
-            if (records.isEmpty() || (attributes = records.get(ldapAttr)) == null) {
+            if (records.isEmpty() || (attributeValues = records.get(ldapAttr)) == null) {
                 return;
             }
 
-            ldapUser.setAttribute(ldapAttr, attributes);
-            sessionAllowed = attributes.stream().anyMatch((t) -> whitelist.contains(t));
+            ldapUser.setAttribute(ldapAttr, attributeValues);
+            sessionAllowed = attributeValues.stream().anyMatch((t) -> whitelist.contains(t));
         }
 
         updateSession(req, sessionAllowed);

plugins/src/opengrok/auth/plugin/LdapFilterPlugin.java

@@ -83,11 +83,6 @@ public void fillSession(HttpServletRequest req, User user) {
             return;
         }
 
-        if (ldapUser.getUid() == null) {
-            LOGGER.log(Level.FINER, "failed to get uid");
-            return;
-        }
-
         String expandedFilter = expandFilter(ldapFilter, ldapUser, user);
         LOGGER.log(Level.FINER, "expanded filter for user {0} into ''{1}''",
                 new Object[]{user, expandedFilter});
@@ -103,30 +98,25 @@ public void fillSession(HttpServletRequest req, User user) {
     }
 
     /**
-     * Insert the special values into the filter.
+     * Expand LdapUser attribute values into the filter.
      *
      * Special values are:
      * <ul>
-     * <li>%uid% - to be replaced with LDAP uid value</li>
-     * <li>%mail% - to be replaced with LDAP mail value</li>
-     * <li>%username% - to be replaced with OSSO user value</li>
-     * <li>%guid% - to be replaced with OSSO guid value</li>
+     * <li>%username% - to be replaced with username value from the User object</li>
+     * <li>%guid% - to be replaced with guid value from the User object</li>
      * </ul>
      *
      * Use \% for printing the '%' character.
-     * Also replaces any other LDAP attribute that would not be ambiguous.
      *
      * @param filter basic filter containing the special values
      * @param ldapUser user from LDAP
      * @param user user from the request
      * @return replaced result
      */
     protected String expandFilter(String filter, LdapUser ldapUser, User user) {
-        filter = filter.replaceAll("(?<!\\\\)%uid(?<!\\\\)%", ldapUser.getUid());
-        filter = filter.replaceAll("(?<!\\\\)%mail(?<!\\\\)%", ldapUser.getMail());
         filter = filter.replaceAll("(?<!\\\\)%username(?<!\\\\)%", user.getUsername());
         filter = filter.replaceAll("(?<!\\\\)%guid(?<!\\\\)%", user.getId());
-        
+
         for (Entry<String, Set<String>> entry : ldapUser.getAttributes().entrySet()) {
             if (entry.getValue().size() == 1) {
                 try {
@@ -137,7 +127,6 @@ protected String expandFilter(String filter, LdapUser ldapUser, User user) {
                     LOGGER.log(Level.WARNING, "The pattern for expanding is not valid", ex);
                 }
             }
-
         }
 
         filter = filter.replaceAll("\\\\%", "%");

plugins/src/opengrok/auth/plugin/LdapUserPlugin.java

@@ -22,6 +22,7 @@
  */
 package opengrok.auth.plugin;
 
+import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
 import java.util.logging.Level;
@@ -37,6 +38,7 @@
 
 /**
  * Authorization plug-in to extract user's LDAP attributes.
+ * The attributes can be then used by the other LDAP plugins down the stack.
  *
  * @author Krystof Tulinger
  */
@@ -45,9 +47,19 @@ public class LdapUserPlugin extends AbstractLdapPlugin {
     private static final Logger LOGGER = Logger.getLogger(LdapUserPlugin.class.getName());
 
     public static final String SESSION_ATTR = "opengrok-ldap-plugin-user";
+
+    /**
+     * configuration names
+     * <ul>
+     * <li><code>objectclass</code> is LDAP object class</li>
+     * <li><code>attributes</code> is comma separated list of LDAP attributes</li>
+     * </ul>
+     */
     protected static final String OBJECT_CLASS = "objectclass";
+    protected static final String ATTRIBUTES = "attributes";
 
     private String objectClass;
+    private String[] attributes;
     private final Pattern usernameCnPattern = Pattern.compile("(cn=[a-zA-Z0-9_-]+)");
 
     @Override
@@ -63,9 +75,17 @@ public void load(Map<String, Object> parameters) {
             throw new NullPointerException("object class '" + objectClass +
                     "' contains non-alphanumeric characters");
         }
-    
-        LOGGER.log(Level.FINE, "LdapUser plugin loaded with objectclass={0}",
-                objectClass);
+
+        String attributesVal;
+        if ((attributesVal = (String) parameters.get(ATTRIBUTES)) == null) {
+            throw new NullPointerException("Missing param [" + ATTRIBUTES +
+                    "] in the setup");
+        }
+        attributes = attributesVal.split(",");
+
+        LOGGER.log(Level.FINE, "LdapUser plugin loaded with objectclass={0}, " +
+                        "attributes={1}",
+                new Object[]{objectClass, String.join(", ", attributes)});
     }
 
     /**
@@ -108,14 +128,15 @@ public void fillSession(HttpServletRequest req, User user) {
         updateSession(req, null);
 
         if (getLdapProvider() == null) {
+            LOGGER.log(Level.WARNING, "cannot get LDAP provider for LdapUser plugin");
             return;
         }
 
         String filter = getFilter(user);
-        if ((records = getLdapProvider().lookupLdapContent(null, filter,
-                new String[]{"uid", "mail", "ou"})) == null) {
-            LOGGER.log(Level.WARNING, "failed to get LDAP contents for user ''{0}'' with filter ''{1}''",
-                    new Object[]{user, filter});
+        if ((records = getLdapProvider().lookupLdapContent(null, filter, attributes)) == null) {
+            LOGGER.log(Level.WARNING, "failed to get LDAP attributes ''{3}'' for user ''{0}'' " +
+                            "with filter ''{1}''",
+                    new Object[]{user, filter, String.join(", ", attributes)});
             return;
         }
 
@@ -125,22 +146,19 @@ public void fillSession(HttpServletRequest req, User user) {
             return;
         }
 
-        if (!records.containsKey("uid") || records.get("uid").isEmpty()) {
-            LOGGER.log(Level.WARNING, "uid record for user {0} is not present or empty",
-                    user);
-            return;
+        for (String attrName : attributes) {
+            if (!records.containsKey(attrName) || records.get(attrName).isEmpty()) {
+                LOGGER.log(Level.WARNING, "{0} record for user {1} is not present or empty",
+                        new Object[]{attrName, user});
+            }
         }
 
-        if (!records.containsKey("mail") || records.get("mail").isEmpty()) {
-            LOGGER.log(Level.WARNING, "mail record for user {0} is not present or empty",
-                    user);
-            return;
+        Map<String, Set<String>> attrSet = new HashMap<>();
+        for (String attrName : attributes) {
+            attrSet.put(attrName, records.get(attrName));
         }
 
-        updateSession(req, new LdapUser(
-                records.get("mail").iterator().next(),
-                records.get("uid").iterator().next(),
-                records.get("ou")));
+        updateSession(req, new LdapUser(attrSet));
     }
 
     /**

plugins/test/opengrok/auth/plugin/LdapAttrPluginTest.java

@@ -29,6 +29,8 @@
 import java.io.Writer;
 import java.nio.file.Files;
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.TreeSet;
@@ -78,11 +80,16 @@ public void setUp() {
         plugin.load(parameters);
     }
 
+    @SuppressWarnings("unchecked")
     private void prepareRequest(String username, String mail, String... ous) {
         dummyRequest = new DummyHttpServletRequestLdap();
-        dummyRequest.setAttribute(UserPlugin.REQUEST_ATTR, new User(username, "123", null, false));
-        dummyRequest.getSession().setAttribute(LdapUserPlugin.SESSION_ATTR, new LdapUser(mail, "123",
-                new TreeSet<>(Arrays.asList(ous))));
+        dummyRequest.setAttribute(UserPlugin.REQUEST_ATTR,
+                new User(username, "123", null, false));
+        LdapUser ldapUser = new LdapUser();
+        ldapUser.setAttribute("mail", new TreeSet<>(Collections.singletonList(mail)));
+        ldapUser.setAttribute("uid", new TreeSet<>(Collections.singletonList("123")));
+        ldapUser.setAttribute("ou", new TreeSet<>(Arrays.asList(ous)));
+        dummyRequest.getSession().setAttribute(LdapUserPlugin.SESSION_ATTR, ldapUser);
         plugin.setSessionEstablished(dummyRequest, true);
         plugin.setSessionUsername(dummyRequest, username);
     }
@@ -104,7 +111,7 @@ private Group makeGroup(String name) {
      */
     @Test
     public void testIsAllowed() {
-        /**
+        /*
          * whitelist[mail] => [[email protected], [email protected], just_a_text]
          */
         prepareRequest("007", "[email protected]", "MI6", "MI7");