diff --git a/pkg/modules/auth/ldap/ldap.go b/pkg/modules/auth/ldap/ldap.go index d0b7d7fbb..589f108f4 100644 --- a/pkg/modules/auth/ldap/ldap.go +++ b/pkg/modules/auth/ldap/ldap.go @@ -21,6 +21,7 @@ import ( "crypto/tls" "errors" "fmt" + "slices" "strings" "code.vikunja.io/api/pkg/config" @@ -229,7 +230,8 @@ func AuthenticateUserInLDAP(s *xorm.Session, username, password string, syncGrou } } - err = syncUserGroups(s, l, u, userdn) + ldapUsername := sr.Entries[0].GetAttributeValue(config.AuthLdapAttributeUsername.GetString()) + err = syncUserGroups(s, l, u, userdn, ldapUsername) return u, err } @@ -293,44 +295,74 @@ func getOrCreateLdapUser(s *xorm.Session, entry *ldap.Entry) (u *user.User, err return } -func syncUserGroups(s *xorm.Session, l *ldap.Conn, u *user.User, userdn string) (err error) { +const ( + groupSyncFilterUserDN = "{userdn}" + groupSyncFilterUsername = "{username}" +) + +// Below AD's default MaxPageSize of 1000. +const groupSearchPageSize = 500 + +// buildGroupSyncFilter returns perUser = true when the template references the +// user, in which case every group matching the filter is a membership. +func buildGroupSyncFilter(template, userDN, username string) (filter string, perUser bool, err error) { + hasUsername := strings.Contains(template, groupSyncFilterUsername) + if !hasUsername && !strings.Contains(template, groupSyncFilterUserDN) { + return template, false, nil + } + + // An empty value would turn e.g. (memberUid={username}*) into (memberUid=*) and grant every group. + if hasUsername && username == "" { + return "", false, fmt.Errorf("group sync filter uses %s but the user has no %s attribute", groupSyncFilterUsername, config.AuthLdapAttributeUsername.GetString()) + } + + return strings.NewReplacer( + groupSyncFilterUserDN, ldap.EscapeFilter(userDN), + groupSyncFilterUsername, ldap.EscapeFilter(username), + ).Replace(template), true, nil +} + +func syncUserGroups(s *xorm.Session, l *ldap.Conn, u *user.User, userdn, ldapUsername string) (err error) { + filter, perUser, err := buildGroupSyncFilter(config.AuthLdapGroupSyncFilter.GetString(), userdn, ldapUsername) + if err != nil { + return err + } + memberAttribute := config.AuthLdapAttributeMemberID.GetString() + + attributes := []string{"cn", "description"} + if !perUser { + attributes = append(attributes, memberAttribute) + } + searchRequest := ldap.NewSearchRequest( config.AuthLdapBaseDN.GetString(), ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, - config.AuthLdapGroupSyncFilter.GetString(), - []string{ - "dn", - "cn", - config.AuthLdapAttributeMemberID.GetString(), - "description", - }, + filter, + attributes, nil, ) - sr, err := l.Search(searchRequest) + sr, err := l.SearchWithPaging(searchRequest, groupSearchPageSize) if err != nil { log.Errorf("Error searching for LDAP groups: %v", err) return err } + log.Debugf("Found %d LDAP groups for user %s with filter %s", len(sr.Entries), userdn, filter) + var teams []*models.Team + isUser := func(member string) bool { return member == userdn || member == u.Username } for _, group := range sr.Entries { - groupName := group.GetAttributeValue("cn") - members := group.GetAttributeValues(config.AuthLdapAttributeMemberID.GetString()) - description := group.GetAttributeValue("description") - - log.Debugf("Group %s has %d members", groupName, len(members)) - - for _, member := range members { - if member == userdn || member == u.Username { - teams = append(teams, &models.Team{ - Name: groupName, - ExternalID: group.DN, - Description: description, - }) - } + if !perUser && !slices.ContainsFunc(group.GetAttributeValues(memberAttribute), isUser) { + continue } + + teams = append(teams, &models.Team{ + Name: group.GetAttributeValue("cn"), + ExternalID: group.DN, + Description: group.GetAttributeValue("description"), + }) } err = models.SyncExternalTeamsForUser(s, u, teams, user.IssuerLDAP, "LDAP") diff --git a/pkg/modules/auth/ldap/ldap_test.go b/pkg/modules/auth/ldap/ldap_test.go index aad9e991d..74d73cfdf 100644 --- a/pkg/modules/auth/ldap/ldap_test.go +++ b/pkg/modules/auth/ldap/ldap_test.go @@ -102,6 +102,7 @@ func TestLdapLogin(t *testing.T) { "issuer": "ldap", "external_id": "cn=git,ou=people,dc=planetexpress,dc=com", }, false) + assertLdapTeamCount(t, user.ID, 2) }) t.Run("should sync groups using service account rebind", func(t *testing.T) { @@ -162,6 +163,56 @@ func TestLdapLogin(t *testing.T) { }, false) }) + t.Run("should sync groups with per-user filter", func(t *testing.T) { + origFilter := config.AuthLdapGroupSyncFilter.GetString() + config.AuthLdapGroupSyncFilter.Set("(&(objectclass=groupOfNames)(member={userdn}))") + defer config.AuthLdapGroupSyncFilter.Set(origFilter) + + tests := []struct { + name string + username string + groups []string + }{ + { + name: "professor", + username: "professor", + groups: []string{ + "admin_staff", + "git", + }, + }, + { + name: "non-ascii dn", + username: "bender", + groups: []string{ + "ship_crew", + "git", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + user, err := AuthenticateUserInLDAP(s, tt.username, tt.username, true, "") + + require.NoError(t, err) + require.NoError(t, s.Commit()) + for _, group := range tt.groups { + db.AssertExists(t, "teams", map[string]interface{}{ + "name": group + " (LDAP)", + "issuer": "ldap", + "external_id": "cn=" + group + ",ou=people,dc=planetexpress,dc=com", + }, false) + } + assertLdapTeamCount(t, user.ID, 2) + }) + } + }) + t.Run("should sync avatar when enabled", func(t *testing.T) { db.LoadAndAssertFixtures(t) s := db.NewSession() @@ -321,3 +372,96 @@ func TestSanitizedUserQueryPreventsInjection(t *testing.T) { }) } } + +func assertLdapTeamCount(t *testing.T, userID int64, expected int64) { + t.Helper() + s := db.NewSession() + defer s.Close() + + count, err := s. + Table("teams"). + Join("INNER", "team_members", "team_members.team_id = teams.id"). + Where("teams.issuer = ? AND team_members.user_id = ?", user2.IssuerLDAP, userID). + Count() + require.NoError(t, err) + assert.Equal(t, expected, count) +} + +func TestBuildGroupSyncFilter(t *testing.T) { + tests := []struct { + name string + template string + userDN string + username string + expectedFilter string + expectedPerUser bool + expectedErr bool + }{ + { + name: "no placeholder", + template: "(&(objectclass=*)(|(objectclass=group)(objectclass=groupOfNames)))", + userDN: "cn=professor,ou=people,dc=planetexpress,dc=com", + username: "professor", + expectedFilter: "(&(objectclass=*)(|(objectclass=group)(objectclass=groupOfNames)))", + }, + { + name: "userdn", + template: "(&(objectclass=groupOfNames)(member={userdn}))", + userDN: "cn=professor,ou=people,dc=planetexpress,dc=com", + expectedFilter: "(&(objectclass=groupOfNames)(member=cn=professor,ou=people,dc=planetexpress,dc=com))", + expectedPerUser: true, + }, + { + name: "username", + template: "(&(objectclass=posixGroup)(memberUid={username}))", + username: "professor", + expectedFilter: "(&(objectclass=posixGroup)(memberUid=professor))", + expectedPerUser: true, + }, + { + name: "both placeholders, repeated", + template: "(|(member={userdn})(memberUid={username})(uniqueMember={userdn}))", + userDN: "cn=a,dc=b", + username: "a", + expectedFilter: "(|(member=cn=a,dc=b)(memberUid=a)(uniqueMember=cn=a,dc=b))", + expectedPerUser: true, + }, + { + name: "injection attempt in username", + template: "(&(objectclass=posixGroup)(memberUid={username}))", + username: "x)(|(objectclass=*", + expectedFilter: `(&(objectclass=posixGroup)(memberUid=x\29\28|\28objectclass=\2a))`, + expectedPerUser: true, + }, + { + name: "placeholder in value is not substituted again", + template: "(|(member={userdn})(memberUid={username}))", + userDN: "cn={username},dc=example", + username: "professor", + expectedFilter: "(|(member=cn={username},dc=example)(memberUid=professor))", + expectedPerUser: true, + }, + { + name: "empty username", + template: "(&(objectclass=posixGroup)(memberUid={username}*))", + userDN: "cn=professor,ou=people,dc=planetexpress,dc=com", + expectedErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + filter, perUser, err := buildGroupSyncFilter(tt.template, tt.userDN, tt.username) + if tt.expectedErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.expectedFilter, filter) + assert.Equal(t, tt.expectedPerUser, perUser) + + _, err = ldap.CompileFilter(filter) + require.NoError(t, err) + }) + } +}