Address additional code review suggestions

- Improve error message clarity in updateUserGroups
- Preallocate slice capacity to avoid reallocations
- Optimize memory usage in group assignment

Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com>

object/ldap_autosync.go

@@ -116,7 +116,7 @@ func (l *LdapAutoSynchronizer) syncRoutine(ldap *Ldap, stopChan chan struct{}) e
 			logs.Warning(fmt.Sprintf("ldap autosync,%d new users,but %d user failed during :", len(users)-len(existed)-len(failed), len(failed)), failed)
 			logs.Warning(err.Error())
 		} else {
-			logs.Info(fmt.Sprintf("ldap autosync success, %d new users, %d existing users", len(users)-len(existed), len(existed)))
+			logs.Info(fmt.Sprintf("ldap autosync success, %d new users, %d existing users (groups synchronized)", len(users)-len(existed), len(existed)))
 		}
 
 		conn.Close()

object/ldap_conn.go

@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/beego/beego/v2/core/logs"
 	"github.com/casdoor/casdoor/conf"
 	"github.com/casdoor/casdoor/i18n"
 	"github.com/casdoor/casdoor/util"
@@ -31,6 +32,10 @@ import (
 	"golang.org/x/text/encoding/unicode"
 )
 
+const (
+	LdapGroupType = "ldap-group"
+)
+
 // formatUserPhone processes phone number for a user based on their CountryCode
 func formatUserPhone(u *User) {
 	if u.Phone == "" {
@@ -88,6 +93,7 @@ type LdapUser struct {
 	GroupId    string            `json:"groupId"`
 	Address    string            `json:"address"`
 	MemberOf   string            `json:"memberOf"`
+	MemberOfs  []string          `json:"memberOfs"`
 	Attributes map[string]string `json:"attributes"`
 }
 
@@ -179,7 +185,7 @@ func (l *LdapConn) GetLdapUsers(ldapServer *Ldap) ([]LdapUser, error) {
 	SearchAttributes := []string{
 		"uidNumber", "cn", "sn", "gidNumber", "entryUUID", "displayName", "mail", "email",
 		"emailAddress", "telephoneNumber", "mobile", "mobileTelephoneNumber", "registeredAddress", "postalAddress",
-		"c", "co",
+		"c", "co", "memberOf",
 	}
 	if l.IsAD {
 		SearchAttributes = append(SearchAttributes, "sAMAccountName")
@@ -248,6 +254,7 @@ func (l *LdapConn) GetLdapUsers(ldapServer *Ldap) ([]LdapUser, error) {
 				user.CountryName = attribute.Values[0]
 			case "memberOf":
 				user.MemberOf = attribute.Values[0]
+				user.MemberOfs = attribute.Values
 			default:
 				if propName, ok := ldapServer.CustomAttributes[attribute.Name]; ok {
 					if user.Attributes == nil {
@@ -315,12 +322,104 @@ func AutoAdjustLdapUser(users []LdapUser) []LdapUser {
 			Address:     util.ReturnAnyNotEmpty(user.Address, user.PostalAddress, user.RegisteredAddress),
 			Country:     util.ReturnAnyNotEmpty(user.Country, user.CountryName),
 			CountryName: user.CountryName,
+			MemberOf:    user.MemberOf,
+			MemberOfs:   user.MemberOfs,
 			Attributes:  user.Attributes,
 		}
 	}
 	return res
 }
 
+// parseGroupNameFromDN extracts the CN (Common Name) from an LDAP DN
+// e.g., "CN=GroupName,OU=Groups,DC=example,DC=com" -> "GroupName"
+func parseGroupNameFromDN(dn string) string {
+	if dn == "" {
+		return ""
+	}
+
+	// Split by comma and find the CN component
+	parts := strings.Split(dn, ",")
+	for _, part := range parts {
+		part = strings.TrimSpace(part)
+		if strings.HasPrefix(strings.ToLower(part), "cn=") {
+			return part[3:] // Return everything after "cn="
+		}
+	}
+	return ""
+}
+
+// extractGroupNamesFromMemberOf extracts group names from memberOf DNs
+func extractGroupNamesFromMemberOf(memberOfs []string) []string {
+	var groupNames []string
+	for _, dn := range memberOfs {
+		groupName := parseGroupNameFromDN(dn)
+		if groupName != "" {
+			groupNames = append(groupNames, groupName)
+		}
+	}
+	return groupNames
+}
+
+// ensureGroupExists creates a group if it doesn't exist
+func ensureGroupExists(owner, groupName string) error {
+	if groupName == "" {
+		return nil
+	}
+
+	existingGroup, err := getGroup(owner, groupName)
+	if err != nil {
+		return err
+	}
+
+	if existingGroup != nil {
+		return nil // Group already exists
+	}
+
+	// Create the group
+	newGroup := &Group{
+		Owner:       owner,
+		Name:        groupName,
+		CreatedTime: util.GetCurrentTime(),
+		UpdatedTime: util.GetCurrentTime(),
+		DisplayName: groupName,
+		Type:        LdapGroupType,
+		IsEnabled:   true,
+		IsTopGroup:  true,
+	}
+
+	_, err = AddGroup(newGroup)
+	return err
+}
+
+// updateUserGroups updates an existing user's group memberships from LDAP
+func updateUserGroups(owner string, syncUser LdapUser, ldapGroupNames []string, defaultGroup string) error {
+	// Find the user by LDAP UUID
+	user := &User{}
+	has, err := ormer.Engine.Where("owner = ? AND ldap = ?", owner, syncUser.Uuid).Get(user)
+	if err != nil {
+		return err
+	}
+	if !has {
+		return fmt.Errorf("user with LDAP UUID %s not found in database (may be a new user)", syncUser.Uuid)
+	}
+
+	// Prepare new group list with preallocated capacity
+	capacity := len(ldapGroupNames)
+	if defaultGroup != "" {
+		capacity++
+	}
+	newGroups := make([]string, 0, capacity)
+	if defaultGroup != "" {
+		newGroups = append(newGroups, defaultGroup)
+	}
+	newGroups = append(newGroups, ldapGroupNames...)
+
+	// Update user groups
+	user.Groups = newGroups
+	_, err = UpdateUser(user.GetId(), user, []string{"groups"}, false)
+	return err
+}
+
 func SyncLdapUsers(owner string, syncUsers []LdapUser, ldapId string) (existUsers []LdapUser, failedUsers []LdapUser, err error) {
 	var uuids []string
 	for _, user := range syncUsers {
@@ -356,12 +455,32 @@ func SyncLdapUsers(owner string, syncUsers []LdapUser, ldapId string) (existUser
 			return nil, nil, err
 		}
 
+		// Extract group names from LDAP memberOf attributes
+		ldapGroupNames := extractGroupNamesFromMemberOf(syncUser.MemberOfs)
+
+		// Ensure all LDAP groups exist in Casdoor
+		for _, groupName := range ldapGroupNames {
+			err := ensureGroupExists(owner, groupName)
+			if err != nil {
+				// Log warning but continue processing
+				logs.Warning("Failed to create LDAP group %s: %v", groupName, err)
+			}
+		}
+
 		found := false
 		if len(existUuids) > 0 {
 			for _, existUuid := range existUuids {
 				if syncUser.Uuid == existUuid {
 					existUsers = append(existUsers, syncUser)
 					found = true
+
+					// Update existing user's group memberships
+					if len(ldapGroupNames) > 0 {
+						err := updateUserGroups(owner, syncUser, ldapGroupNames, ldap.DefaultGroup)
+						if err != nil {
+							logs.Warning("Failed to update groups for user %s: %v", syncUser.Uuid, err)
+						}
+					}
 				}
 			}
 		}
@@ -377,6 +496,18 @@ func SyncLdapUsers(owner string, syncUsers []LdapUser, ldapId string) (existUser
 				return nil, nil, err
 			}
 
+			// Prepare group assignments for new user with preallocated capacity
+			capacity := len(ldapGroupNames)
+			if ldap.DefaultGroup != "" {
+				capacity++
+			}
+			userGroups := make([]string, 0, capacity)
+			if ldap.DefaultGroup != "" {
+				userGroups = append(userGroups, ldap.DefaultGroup)
+			}
+			// Add LDAP groups
+			userGroups = append(userGroups, ldapGroupNames...)
+
 			newUser := &User{
 				Owner:             owner,
 				Name:              name,
@@ -395,13 +526,10 @@ func SyncLdapUsers(owner string, syncUsers []LdapUser, ldapId string) (existUser
 				Score:             score,
 				Ldap:              syncUser.Uuid,
 				Properties:        syncUser.Attributes,
+				Groups:            userGroups,
 			}
 			formatUserPhone(newUser)
 
-			if ldap.DefaultGroup != "" {
-				newUser.Groups = []string{ldap.DefaultGroup}
-			}
-
 			affected, err := AddUser(newUser, "en")
 			if err != nil {
 				return nil, nil, err

object/ldap_conn_test.go

@@ -0,0 +1,143 @@
+// Copyright 2025 The Casdoor Authors. 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 object
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestParseGroupNameFromDN(t *testing.T) {
+	testCases := []struct {
+		name     string
+		dn       string
+		expected string
+	}{
+		{
+			name:     "Standard Active Directory DN",
+			dn:       "CN=Domain Admins,OU=Groups,DC=example,DC=com",
+			expected: "Domain Admins",
+		},
+		{
+			name:     "OpenLDAP DN",
+			dn:       "cn=developers,ou=groups,dc=example,dc=org",
+			expected: "developers",
+		},
+		{
+			name:     "DN with spaces",
+			dn:       "CN=Project Managers,OU=Marketing,DC=company,DC=net",
+			expected: "Project Managers",
+		},
+		{
+			name:     "DN with special characters",
+			dn:       "CN=IT-Support-Team,OU=IT,DC=domain,DC=local",
+			expected: "IT-Support-Team",
+		},
+		{
+			name:     "Empty DN",
+			dn:       "",
+			expected: "",
+		},
+		{
+			name:     "DN without CN",
+			dn:       "OU=Groups,DC=example,DC=com",
+			expected: "",
+		},
+		{
+			name:     "DN with extra spaces",
+			dn:       "CN=Engineers,OU=Tech,DC=example,DC=com",
+			expected: "Engineers",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := parseGroupNameFromDN(tc.dn)
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}
+
+func TestExtractGroupNamesFromMemberOf(t *testing.T) {
+	testCases := []struct {
+		name      string
+		memberOfs []string
+		expected  []string
+	}{
+		{
+			name: "Multiple groups from Active Directory",
+			memberOfs: []string{
+				"CN=Domain Admins,OU=Groups,DC=example,DC=com",
+				"CN=IT Support,OU=Groups,DC=example,DC=com",
+				"CN=Developers,OU=Engineering,DC=example,DC=com",
+			},
+			expected: []string{"Domain Admins", "IT Support", "Developers"},
+		},
+		{
+			name: "Single group from OpenLDAP",
+			memberOfs: []string{
+				"cn=developers,ou=groups,dc=example,dc=org",
+			},
+			expected: []string{"developers"},
+		},
+		{
+			name:      "Empty memberOf list",
+			memberOfs: []string{},
+			expected:  nil,
+		},
+		{
+			name: "Mixed valid and invalid DNs",
+			memberOfs: []string{
+				"CN=Valid Group,OU=Groups,DC=example,DC=com",
+				"OU=InvalidDN,DC=example,DC=com",
+				"CN=Another Valid,OU=Teams,DC=example,DC=com",
+			},
+			expected: []string{"Valid Group", "Another Valid"},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := extractGroupNamesFromMemberOf(tc.memberOfs)
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}
+
+func TestAutoAdjustLdapUser_WithMemberOf(t *testing.T) {
+	// Test that AutoAdjustLdapUser properly preserves MemberOfs
+	users := []LdapUser{
+		{
+			Uid:         "testuser",
+			Cn:          "Test User",
+			DisplayName: "Test User",
+			Email:       "test@example.com",
+			MemberOf:    "CN=Admins,OU=Groups,DC=example,DC=com",
+			MemberOfs: []string{
+				"CN=Admins,OU=Groups,DC=example,DC=com",
+				"CN=Developers,OU=Groups,DC=example,DC=com",
+			},
+		},
+	}
+
+	result := AutoAdjustLdapUser(users)
+
+	assert.Len(t, result, 1)
+	assert.Equal(t, "CN=Admins,OU=Groups,DC=example,DC=com", result[0].MemberOf)
+	assert.Len(t, result[0].MemberOfs, 2)
+	assert.Equal(t, "CN=Admins,OU=Groups,DC=example,DC=com", result[0].MemberOfs[0])
+	assert.Equal(t, "CN=Developers,OU=Groups,DC=example,DC=com", result[0].MemberOfs[1])
+}