Merge pull request #903 from ericchiang/ldap-groups-on-user

connector/ldap: fix case where groups are listed on the user entity

connector/ldap/ldap.go

@@ -256,18 +256,22 @@ func (c *ldapConnector) do(ctx context.Context, f func(c *ldap.Conn) error) erro
 	return f(conn)
 }
 
-func getAttr(e ldap.Entry, name string) string {
+func getAttrs(e ldap.Entry, name string) []string {
 	for _, a := range e.Attributes {
 		if a.Name != name {
 			continue
 		}
-		if len(a.Values) == 0 {
+		return a.Values
-			return ""
-		}
-		return a.Values[0]
 	}
 	if name == "DN" {
-		return e.DN
+		return []string{e.DN}
+	}
+	return nil
+}
+
+func getAttr(e ldap.Entry, name string) string {
+	if a := getAttrs(e, name); len(a) > 0 {
+		return a[0]
 	}
 	return ""
 }
@@ -454,36 +458,39 @@ func (c *ldapConnector) groups(ctx context.Context, user ldap.Entry) ([]string,
 		return nil, nil
 	}
 
-	filter := fmt.Sprintf("(%s=%s)", c.GroupSearch.GroupAttr, ldap.EscapeFilter(getAttr(user, c.GroupSearch.UserAttr)))
-	if c.GroupSearch.Filter != "" {
-		filter = fmt.Sprintf("(&%s%s)", c.GroupSearch.Filter, filter)
-	}
-
-	req := &ldap.SearchRequest{
-		BaseDN:     c.GroupSearch.BaseDN,
-		Filter:     filter,
-		Scope:      c.groupSearchScope,
-		Attributes: []string{c.GroupSearch.NameAttr},
-	}
-
 	var groups []*ldap.Entry
-	if err := c.do(ctx, func(conn *ldap.Conn) error {
+	for _, attr := range getAttrs(user, c.GroupSearch.UserAttr) {
-		resp, err := conn.Search(req)
+		filter := fmt.Sprintf("(%s=%s)", c.GroupSearch.GroupAttr, ldap.EscapeFilter(attr))
-		if err != nil {
+		if c.GroupSearch.Filter != "" {
-			return fmt.Errorf("ldap: search failed: %v", err)
+			filter = fmt.Sprintf("(&%s%s)", c.GroupSearch.Filter, filter)
+		}
+
+		req := &ldap.SearchRequest{
+			BaseDN:     c.GroupSearch.BaseDN,
+			Filter:     filter,
+			Scope:      c.groupSearchScope,
+			Attributes: []string{c.GroupSearch.NameAttr},
+		}
+
+		gotGroups := false
+		if err := c.do(ctx, func(conn *ldap.Conn) error {
+			resp, err := conn.Search(req)
+			if err != nil {
+				return fmt.Errorf("ldap: search failed: %v", err)
+			}
+			gotGroups = len(resp.Entries) != 0
+			groups = append(groups, resp.Entries...)
+			return nil
+		}); err != nil {
+			return nil, err
+		}
+		if !gotGroups {
+			// TODO(ericchiang): Is this going to spam the logs?
+			c.logger.Errorf("ldap: groups search with filter %q returned no groups", filter)
 		}
-		groups = resp.Entries
-		return nil
-	}); err != nil {
-		return nil, err
-	}
-	if len(groups) == 0 {
-		// TODO(ericchiang): Is this going to spam the logs?
-		c.logger.Errorf("ldap: groups search with filter %q returned no groups", filter)
 	}
 
 	var groupNames []string
-
 	for _, group := range groups {
 		name := getAttr(*group, c.GroupSearch.NameAttr)
 		if name == "" {

connector/ldap/ldap_test.go

@@ -52,7 +52,7 @@ ou: People
 
 dn: cn=jane,ou=People,dc=example,dc=org
 objectClass: person
-objectClass: iNetOrgPerson
+objectClass: inetOrgPerson
 sn: doe
 cn: jane
 mail: janedoe@example.com
@@ -60,7 +60,7 @@ userpassword: foo
 
 dn: cn=john,ou=People,dc=example,dc=org
 objectClass: person
-objectClass: iNetOrgPerson
+objectClass: inetOrgPerson
 sn: doe
 cn: john
 mail: johndoe@example.com
@@ -127,7 +127,7 @@ ou: People
 
 dn: cn=jane,ou=People,dc=example,dc=org
 objectClass: person
-objectClass: iNetOrgPerson
+objectClass: inetOrgPerson
 sn: doe
 cn: jane
 mail: janedoe@example.com
@@ -135,7 +135,7 @@ userpassword: foo
 
 dn: cn=john,ou=People,dc=example,dc=org
 objectClass: person
-objectClass: iNetOrgPerson
+objectClass: inetOrgPerson
 sn: doe
 cn: john
 mail: johndoe@example.com
@@ -201,6 +201,103 @@ member: cn=jane,ou=People,dc=example,dc=org
 	runTests(t, schema, c, tests)
 }
 
+func TestGroupsOnUserEntity(t *testing.T) {
+	schema := `
+dn: dc=example,dc=org
+objectClass: dcObject
+objectClass: organization
+o: Example Company
+dc: example
+
+dn: ou=People,dc=example,dc=org
+objectClass: organizationalUnit
+ou: People
+
+# Groups are enumerated as part of the user entity instead of the members being
+# a list on the group entity.
+
+dn: cn=jane,ou=People,dc=example,dc=org
+objectClass: person
+objectClass: inetOrgPerson
+sn: doe
+cn: jane
+mail: janedoe@example.com
+userpassword: foo
+departmentNumber: 1000
+departmentNumber: 1001
+
+dn: cn=john,ou=People,dc=example,dc=org
+objectClass: person
+objectClass: inetOrgPerson
+sn: doe
+cn: john
+mail: johndoe@example.com
+userpassword: bar
+departmentNumber: 1000
+departmentNumber: 1002
+
+# Group definitions. Notice that they don't have any "member" field.
+
+dn: ou=Groups,dc=example,dc=org
+objectClass: organizationalUnit
+ou: Groups
+
+dn: cn=admins,ou=Groups,dc=example,dc=org
+objectClass: posixGroup
+cn: admins
+gidNumber: 1000
+
+dn: cn=developers,ou=Groups,dc=example,dc=org
+objectClass: posixGroup
+cn: developers
+gidNumber: 1001
+
+dn: cn=designers,ou=Groups,dc=example,dc=org
+objectClass: posixGroup
+cn: designers
+gidNumber: 1002
+`
+	c := &Config{}
+	c.UserSearch.BaseDN = "ou=People,dc=example,dc=org"
+	c.UserSearch.NameAttr = "cn"
+	c.UserSearch.EmailAttr = "mail"
+	c.UserSearch.IDAttr = "DN"
+	c.UserSearch.Username = "cn"
+	c.GroupSearch.BaseDN = "ou=Groups,dc=example,dc=org"
+	c.GroupSearch.UserAttr = "departmentNumber"
+	c.GroupSearch.GroupAttr = "gidNumber"
+	c.GroupSearch.NameAttr = "cn"
+	tests := []subtest{
+		{
+			name:     "validpassword",
+			username: "jane",
+			password: "foo",
+			groups:   true,
+			want: connector.Identity{
+				UserID:        "cn=jane,ou=People,dc=example,dc=org",
+				Username:      "jane",
+				Email:         "janedoe@example.com",
+				EmailVerified: true,
+				Groups:        []string{"admins", "developers"},
+			},
+		},
+		{
+			name:     "validpassword2",
+			username: "john",
+			password: "bar",
+			groups:   true,
+			want: connector.Identity{
+				UserID:        "cn=john,ou=People,dc=example,dc=org",
+				Username:      "john",
+				Email:         "johndoe@example.com",
+				EmailVerified: true,
+				Groups:        []string{"admins", "designers"},
+			},
+		},
+	}
+	runTests(t, schema, c, tests)
+}
+
 // runTests runs a set of tests against an LDAP schema. It does this by
 // setting up an OpenLDAP server and injecting the provided scheme.
 //