Merge pull request #5164 from stonezdj/fix_invalid_dn_500

Handle Invalid syntax and not found error

docs/swagger.yaml

@@ -2441,6 +2441,8 @@ paths:
       responses:
         '201':
           description: User group created successfully.
+        '400':
+          description: Invalid LDAP group DN.
         '401':
           description: User need to log in first.
         '403':

src/common/utils/ldap/ldap.go

@@ -16,6 +16,7 @@ package ldap
 
 import (
 	"crypto/tls"
+	"errors"
 	"fmt"
 	"net/url"
 	"strconv"
@@ -29,10 +30,17 @@ import (
 	goldap "gopkg.in/ldap.v2"
 )
 
+//ErrNotFound ...
+var ErrNotFound = errors.New("entity not found")
+
+//ErrDNSyntax ...
+var ErrDNSyntax = errors.New("Invalid DN syntax")
+
 //Session - define a LDAP session
 type Session struct {
-	ldapConfig models.LdapConf
-	ldapConn   *goldap.Conn
+	ldapConfig      models.LdapConf
+	ldapGroupConfig models.LdapGroupConf
+	ldapConn        *goldap.Conn
 }
 
 //LoadSystemLdapConfig - load LDAP configure from adminserver
@@ -53,11 +61,23 @@ func LoadSystemLdapConfig() (*Session, error) {
 	if err != nil {
 		return nil, err
 	}
-	return CreateWithConfig(*ldapConf)
+
+	ldapGroupConfig, err := config.LDAPGroupConf()
+
+	if err != nil {
+		return nil, err
+	}
+
+	return CreateWithAllConfig(*ldapConf, *ldapGroupConfig)
 }
 
-// CreateWithConfig - create a Session with internal config
+//CreateWithConfig -
 func CreateWithConfig(ldapConf models.LdapConf) (*Session, error) {
+	return CreateWithAllConfig(ldapConf, models.LdapGroupConf{})
+}
+
+// CreateWithAllConfig - create a Session with internal config
+func CreateWithAllConfig(ldapConf models.LdapConf, ldapGroupConfig models.LdapGroupConf) (*Session, error) {
 	var session Session
 
 	if ldapConf.LdapURL == "" {
@@ -71,6 +91,7 @@ func CreateWithConfig(ldapConf models.LdapConf) (*Session, error) {
 
 	ldapConf.LdapURL = ldapURL
 	session.ldapConfig = ldapConf
+	session.ldapGroupConfig = ldapGroupConfig
 	return &session, nil
 }
 
@@ -126,11 +147,16 @@ func (session *Session) ConnectionTest() error {
 		return fmt.Errorf("Failed to load system ldap config")
 	}
 
-	return ConnectionTestWithConfig(session.ldapConfig)
+	return ConnectionTestWithAllConfig(session.ldapConfig, session.ldapGroupConfig)
 }
 
-//ConnectionTestWithConfig - test ldap session connection, out of the scope of normal session create/close
+//ConnectionTestWithConfig -
 func ConnectionTestWithConfig(ldapConfig models.LdapConf) error {
+	return ConnectionTestWithAllConfig(ldapConfig, models.LdapGroupConf{})
+}
+
+//ConnectionTestWithAllConfig - test ldap session connection, out of the scope of normal session create/close
+func ConnectionTestWithAllConfig(ldapConfig models.LdapConf, ldapGroupConfig models.LdapGroupConf) error {
 
 	authMode, err := config.AuthMode()
 	if err != nil {
@@ -150,7 +176,7 @@ func ConnectionTestWithConfig(ldapConfig models.LdapConf) error {
 		ldapConfig.LdapSearchPassword = session.ldapConfig.LdapSearchPassword
 	}
 
-	testSession, err := CreateWithConfig(ldapConfig)
+	testSession, err := CreateWithAllConfig(ldapConfig, ldapGroupConfig)
 
 	if err != nil {
 		return err
@@ -336,22 +362,25 @@ func (session *Session) Close() {
 
 //SearchGroupByName ...
 func (session *Session) SearchGroupByName(groupName string) ([]models.LdapGroup, error) {
-	ldapGroupConfig, err := config.LDAPGroupConf()
-	log.Debugf("Ldap group config: %+v", ldapGroupConfig)
-	if err != nil {
-		return nil, err
-	}
-	return session.searchGroup(ldapGroupConfig.LdapGroupBaseDN, ldapGroupConfig.LdapGroupFilter, groupName, ldapGroupConfig.LdapGroupNameAttribute)
+	return session.searchGroup(session.ldapGroupConfig.LdapGroupBaseDN,
+		session.ldapGroupConfig.LdapGroupFilter,
+		groupName,
+		session.ldapGroupConfig.LdapGroupNameAttribute)
 }
 
 //SearchGroupByDN ...
 func (session *Session) SearchGroupByDN(groupDN string) ([]models.LdapGroup, error) {
-	ldapGroupConfig, err := config.LDAPGroupConf()
-	log.Debugf("Ldap group config: %+v", ldapGroupConfig)
-	if err != nil {
-		return nil, err
+	if _, err := goldap.ParseDN(groupDN); err != nil {
+		return nil, ErrDNSyntax
 	}
-	return session.searchGroup(groupDN, ldapGroupConfig.LdapGroupFilter, "", ldapGroupConfig.LdapGroupNameAttribute)
+	groupList, err := session.searchGroup(groupDN, session.ldapGroupConfig.LdapGroupFilter, "", session.ldapGroupConfig.LdapGroupNameAttribute)
+	if serverError, ok := err.(*goldap.Error); ok {
+		log.Debugf("resultCode:%v", serverError.ResultCode)
+	}
+	if err != nil && goldap.IsErrorWithCode(err, goldap.LDAPResultNoSuchObject) {
+		return nil, ErrNotFound
+	}
+	return groupList, err
 }
 
 func (session *Session) searchGroup(baseDN, filter, groupName, groupNameAttribute string) ([]models.LdapGroup, error) {

src/common/utils/ldap/ldap_test.go

@@ -1,16 +1,3 @@
-// Copyright (c) 2017 VMware, Inc. All Rights Reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
 package ldap
 
 import (
@@ -338,3 +325,66 @@ func TestSession_SearchGroup(t *testing.T) {
 		})
 	}
 }
+
+func TestSession_SearchGroupByDN(t *testing.T) {
+	ldapConfig := models.LdapConf{
+		LdapURL:            adminServerLdapTestConfig[common.LDAPURL].(string) + ":389",
+		LdapSearchDn:       adminServerLdapTestConfig[common.LDAPSearchDN].(string),
+		LdapScope:          2,
+		LdapSearchPassword: adminServerLdapTestConfig[common.LDAPSearchPwd].(string),
+		LdapBaseDn:         adminServerLdapTestConfig[common.LDAPBaseDN].(string),
+	}
+	ldapGroupConfig := models.LdapGroupConf{
+		LdapGroupBaseDN:        "ou=group,dc=example,dc=com",
+		LdapGroupFilter:        "objectclass=groupOfNames",
+		LdapGroupNameAttribute: "cn",
+		LdapGroupSearchScope:   2,
+	}
+	type fields struct {
+		ldapConfig      models.LdapConf
+		ldapGroupConfig models.LdapGroupConf
+		ldapConn        *goldap.Conn
+	}
+	type args struct {
+		groupDN string
+	}
+	tests := []struct {
+		name    string
+		fields  fields
+		args    args
+		want    []models.LdapGroup
+		wantErr bool
+	}{
+		{"normal search",
+			fields{ldapConfig: ldapConfig, ldapGroupConfig: ldapGroupConfig},
+			args{groupDN: "cn=harbor_users,ou=groups,dc=example,dc=com"},
+			[]models.LdapGroup{models.LdapGroup{GroupName: "harbor_users", GroupDN: "cn=harbor_users,ou=groups,dc=example,dc=com"}}, false},
+		{"search non-exist group",
+			fields{ldapConfig: ldapConfig, ldapGroupConfig: ldapGroupConfig},
+			args{groupDN: "cn=harbor_non_users,ou=groups,dc=example,dc=com"},
+			nil, true},
+		{"search invalid group dn",
+			fields{ldapConfig: ldapConfig, ldapGroupConfig: ldapGroupConfig},
+			args{groupDN: "random string"},
+			nil, true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			session := &Session{
+				ldapConfig:      tt.fields.ldapConfig,
+				ldapGroupConfig: tt.fields.ldapGroupConfig,
+				ldapConn:        tt.fields.ldapConn,
+			}
+			session.Open()
+			defer session.Close()
+			got, err := session.SearchGroupByDN(tt.args.groupDN)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("Session.SearchGroupByDN() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("Session.SearchGroupByDN() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

src/ui/api/usergroup.go

@@ -22,6 +22,7 @@ import (
 	"github.com/vmware/harbor/src/common"
 	"github.com/vmware/harbor/src/common/dao/group"
 	"github.com/vmware/harbor/src/common/models"
+	"github.com/vmware/harbor/src/common/utils/ldap"
 	"github.com/vmware/harbor/src/common/utils/log"
 	"github.com/vmware/harbor/src/ui/auth"
 )
@@ -105,14 +106,19 @@ func (uga *UserGroupAPI) Post() {
 	}
 	// User can not add ldap group when the ldap server is offline
 	ldapGroup, err := auth.SearchGroup(userGroup.LdapGroupDN)
+	if err == ldap.ErrNotFound || ldapGroup == nil {
+		uga.HandleNotFound(fmt.Sprintf("LDAP Group DN is not found: DN:%v", userGroup.LdapGroupDN))
+		return
+	}
+	if err == ldap.ErrDNSyntax {
+		uga.HandleBadRequest(fmt.Sprintf("Invalid DN syntax. DN: %v", userGroup.LdapGroupDN))
+		return
+	}
 	if err != nil {
 		uga.HandleInternalServerError(fmt.Sprintf("Error occurred in search user group. error: %v", err))
 		return
 	}
-	if ldapGroup == nil {
-		uga.HandleNotFound("The LDAP group is not found")
-		return
-	}
+
 	groupID, err := group.AddUserGroup(userGroup)
 	if err != nil {
 		uga.HandleInternalServerError(fmt.Sprintf("Error occurred in add user group, error: %v", err))