From 20526c5c521c8006dcc56a3c9cd2055ebbade85e Mon Sep 17 00:00:00 2001
From: stonezdj <stonezdj@gmail.com>
Date: Sat, 22 Jan 2022 14:47:44 +0800
Subject: [PATCH] Group members lose access to push or see projects on Harbor  
  Handle the case if there is duplicate user group name when onboard ldap user
 group    Continue to attach groups when it fail on one item    Fixes #16220

Signed-off-by: stonezdj <stonezdj@gmail.com>
---
 src/pkg/usergroup/manager.go      | 32 +++++++++++++++++++++++--
 src/pkg/usergroup/manager_test.go | 40 +++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/src/pkg/usergroup/manager.go b/src/pkg/usergroup/manager.go
index cb5423277..10f22aa0e 100644
--- a/src/pkg/usergroup/manager.go
+++ b/src/pkg/usergroup/manager.go
@@ -19,6 +19,7 @@ import (
 	"errors"
 	"github.com/goharbor/harbor/src/common"
 	"github.com/goharbor/harbor/src/common/utils"
+	"github.com/goharbor/harbor/src/lib/log"
 	"github.com/goharbor/harbor/src/lib/q"
 	"github.com/goharbor/harbor/src/pkg/usergroup/dao"
 	"github.com/goharbor/harbor/src/pkg/usergroup/model"
@@ -83,7 +84,9 @@ func (m *manager) Populate(ctx context.Context, userGroups []model.UserGroup) ([
 	for _, group := range userGroups {
 		err := m.Onboard(ctx, &group)
 		if err != nil {
-			return ugList, err
+			// log the current error and continue
+			log.Warningf("failed to onboard user group %+v, error %v, continue with other user groups", group, err)
+			continue
 		}
 		if group.ID > 0 {
 			ugList = append(ugList, group.ID)
@@ -102,10 +105,35 @@ func (m *manager) UpdateName(ctx context.Context, id int, groupName string) erro
 
 func (m *manager) Onboard(ctx context.Context, g *model.UserGroup) error {
 	if g.GroupType == common.LDAPGroupType {
-		return m.onBoardCommonUserGroup(ctx, g, "LdapGroupDN", "GroupType")
+		return m.onBoardLdapUserGroup(ctx, g)
 	}
 	return m.onBoardCommonUserGroup(ctx, g, "GroupName", "GroupType")
 }
+
+// onBoardLdapUserGroup -- Check if the ldap group name duplicated and onboard the ldap group
+func (m *manager) onBoardLdapUserGroup(ctx context.Context, g *model.UserGroup) error {
+	g.LdapGroupDN = utils.TrimLower(g.LdapGroupDN)
+	// check if any duplicate ldap group name exist
+	ug, err := m.dao.Query(ctx, q.New(q.KeyWords{"GroupName": g.GroupName, "GroupType": g.GroupType}))
+	if err != nil {
+		return err
+	}
+	if len(ug) > 0 {
+		if g.LdapGroupDN == ug[0].LdapGroupDN {
+			g.ID = ug[0].ID
+			return nil
+		}
+		// if duplicated with name, fall back to ldap group dn
+		if len(g.LdapGroupDN) <= 255 {
+			g.GroupName = g.LdapGroupDN
+		} else {
+			g.GroupName = g.LdapGroupDN[:254]
+		}
+		log.Warningf("existing duplicate user group with the same name, name the current user group with ldap group DN %v", g.GroupName)
+	}
+	return m.onBoardCommonUserGroup(ctx, g, "LdapGroupDN", "GroupType")
+}
+
 func (m *manager) onBoardCommonUserGroup(ctx context.Context, g *model.UserGroup, keyAttribute string, combinedKeyAttributes ...string) error {
 	g.LdapGroupDN = utils.TrimLower(g.LdapGroupDN)
 	created, ID, err := m.dao.ReadOrCreate(ctx, g, keyAttribute, combinedKeyAttributes...)
diff --git a/src/pkg/usergroup/manager_test.go b/src/pkg/usergroup/manager_test.go
index e318bb813..2dde32429 100644
--- a/src/pkg/usergroup/manager_test.go
+++ b/src/pkg/usergroup/manager_test.go
@@ -47,6 +47,46 @@ func (s *ManagerTestSuite) TestOnboardGroup() {
 	s.True(len(ugs) > 0)
 }
 
+func (s *ManagerTestSuite) TestOnboardGroupWithDuplicatedName() {
+	ctx := s.Context()
+	ugs := []*model.UserGroup{
+		{
+			GroupName:   "harbor_dev",
+			GroupType:   1,
+			LdapGroupDN: "cn=harbor_dev,ou=groups,dc=example,dc=com",
+		},
+		{
+			GroupName:   "harbor_dev",
+			GroupType:   1,
+			LdapGroupDN: "cn=harbor_dev,ou=groups,dc=example2,dc=com",
+		},
+		{
+			GroupName:   "harbor_dev",
+			GroupType:   1,
+			LdapGroupDN: "cn=harbor_dev,ou=groups,dc=example3,dc=com,dc=verylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcname",
+		},
+	}
+	for _, ug := range ugs {
+		err := s.mgr.Onboard(ctx, ug)
+		s.Nil(err)
+	}
+	// both user group should be onboard to user group
+	ugs, err := s.mgr.List(ctx, q.New(q.KeyWords{"GroupType": 1, "LdapGroupDN": "cn=harbor_dev,ou=groups,dc=example,dc=com"}))
+	s.Nil(err)
+	s.True(len(ugs) > 0)
+
+	ugs, err = s.mgr.List(ctx, q.New(q.KeyWords{"GroupType": 1, "LdapGroupDN": "cn=harbor_dev,ou=groups,dc=example2,dc=com"}))
+	s.Nil(err)
+	s.True(len(ugs) > 0)
+	s.Equal("cn=harbor_dev,ou=groups,dc=example2,dc=com", ugs[0].GroupName)
+
+	ugs, err = s.mgr.List(ctx, q.New(q.KeyWords{"GroupType": 1, "LdapGroupDN": "cn=harbor_dev,ou=groups,dc=example3,dc=com,dc=verylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcname"}))
+	s.Nil(err)
+	s.True(len(ugs) > 0)
+	s.Equal("cn=harbor_dev,ou=groups,dc=example3,dc=com,dc=verylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcnameverylongdcna", ugs[0].GroupName)
+
+}
+
 func (s *ManagerTestSuite) TestPopulateGroup() {
 	ctx := s.Context()
 	ugs := []model.UserGroup{