Fix group filter for ldap source sync (#22506)

There are 2 separate flows of creating a user: authentication and source
sync.
When a group filter is defined, source sync ignores group filter, while
authentication respects it.
With this PR I've fixed this behavior, so both flows now apply this
filter when searching users in LDAP in a unified way.

- Unified LDAP group membership lookup for authentication and source
sync flows
- Replaced custom group membership lookup (used for authentication flow)
with an existing listLdapGroupMemberships method (used for source sync
flow)
- Modified listLdapGroupMemberships and getUserAttributeListedInGroup in
a way group lookup could be called separately
- Added user filtering based on a group membership for a source sync
- Added tests to cover this logic

Co-authored-by: Pavel Ezhov <paejov@gmail.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
This commit is contained in:
Pavel Ezhov 2023-02-02 10:45:00 +03:00 committed by GitHub
parent 9ef8bfb69b
commit 98770d3db8
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 160 additions and 78 deletions

View file

@ -196,22 +196,39 @@ func checkRestricted(l *ldap.Conn, ls *Source, userDN string) bool {
} }
// List all group memberships of a user // List all group memberships of a user
func (source *Source) listLdapGroupMemberships(l *ldap.Conn, uid string) []string { func (source *Source) listLdapGroupMemberships(l *ldap.Conn, uid string, applyGroupFilter bool) []string {
var ldapGroups []string var ldapGroups []string
groupFilter := fmt.Sprintf("(%s=%s)", source.GroupMemberUID, ldap.EscapeFilter(uid)) var searchFilter string
groupFilter, ok := source.sanitizedGroupFilter(source.GroupFilter)
if !ok {
return ldapGroups
}
groupDN, ok := source.sanitizedGroupDN(source.GroupDN)
if !ok {
return ldapGroups
}
if applyGroupFilter {
searchFilter = fmt.Sprintf("(&(%s)(%s=%s))", groupFilter, source.GroupMemberUID, ldap.EscapeFilter(uid))
} else {
searchFilter = fmt.Sprintf("(%s=%s)", source.GroupMemberUID, ldap.EscapeFilter(uid))
}
result, err := l.Search(ldap.NewSearchRequest( result, err := l.Search(ldap.NewSearchRequest(
source.GroupDN, groupDN,
ldap.ScopeWholeSubtree, ldap.ScopeWholeSubtree,
ldap.NeverDerefAliases, ldap.NeverDerefAliases,
0, 0,
0, 0,
false, false,
groupFilter, searchFilter,
[]string{}, []string{},
nil, nil,
)) ))
if err != nil { if err != nil {
log.Error("Failed group search using filter[%s]: %v", groupFilter, err) log.Error("Failed group search in LDAP with filter [%s]: %v", searchFilter, err)
return ldapGroups return ldapGroups
} }
@ -238,9 +255,7 @@ func (source *Source) mapLdapGroupsToTeams() map[string]map[string][]string {
} }
// getMappedMemberships : returns the organizations and teams to modify the users membership // getMappedMemberships : returns the organizations and teams to modify the users membership
func (source *Source) getMappedMemberships(l *ldap.Conn, uid string) (map[string][]string, map[string][]string) { func (source *Source) getMappedMemberships(usersLdapGroups []string, uid string) (map[string][]string, map[string][]string) {
// get all LDAP group memberships for user
usersLdapGroups := source.listLdapGroupMemberships(l, uid)
// unmarshall LDAP group team map from configs // unmarshall LDAP group team map from configs
ldapGroupsToTeams := source.mapLdapGroupsToTeams() ldapGroupsToTeams := source.mapLdapGroupsToTeams()
membershipsToAdd := map[string][]string{} membershipsToAdd := map[string][]string{}
@ -260,6 +275,14 @@ func (source *Source) getMappedMemberships(l *ldap.Conn, uid string) (map[string
return membershipsToAdd, membershipsToRemove return membershipsToAdd, membershipsToRemove
} }
func (source *Source) getUserAttributeListedInGroup(entry *ldap.Entry) string {
if strings.ToLower(source.UserUID) == "dn" {
return entry.DN
}
return entry.GetAttributeValue(source.UserUID)
}
// SearchEntry : search an LDAP source if an entry (name, passwd) is valid and in the specific filter // SearchEntry : search an LDAP source if an entry (name, passwd) is valid and in the specific filter
func (source *Source) SearchEntry(name, passwd string, directBind bool) *SearchResult { func (source *Source) SearchEntry(name, passwd string, directBind bool) *SearchResult {
// See https://tools.ietf.org/search/rfc4513#section-5.1.2 // See https://tools.ietf.org/search/rfc4513#section-5.1.2
@ -375,58 +398,30 @@ func (source *Source) SearchEntry(name, passwd string, directBind bool) *SearchR
firstname := sr.Entries[0].GetAttributeValue(source.AttributeName) firstname := sr.Entries[0].GetAttributeValue(source.AttributeName)
surname := sr.Entries[0].GetAttributeValue(source.AttributeSurname) surname := sr.Entries[0].GetAttributeValue(source.AttributeSurname)
mail := sr.Entries[0].GetAttributeValue(source.AttributeMail) mail := sr.Entries[0].GetAttributeValue(source.AttributeMail)
uid := sr.Entries[0].GetAttributeValue(source.UserUID)
if source.UserUID == "dn" || source.UserUID == "DN" { teamsToAdd := make(map[string][]string)
uid = sr.Entries[0].DN teamsToRemove := make(map[string][]string)
}
// Check group membership // Check group membership
if source.GroupsEnabled && source.GroupFilter != "" { if source.GroupsEnabled {
groupFilter, ok := source.sanitizedGroupFilter(source.GroupFilter) userAttributeListedInGroup := source.getUserAttributeListedInGroup(sr.Entries[0])
if !ok { usersLdapGroups := source.listLdapGroupMemberships(l, userAttributeListedInGroup, true)
return nil
} if source.GroupFilter != "" && len(usersLdapGroups) == 0 {
groupDN, ok := source.sanitizedGroupDN(source.GroupDN)
if !ok {
return nil return nil
} }
log.Trace("Fetching groups '%v' with filter '%s' and base '%s'", source.GroupMemberUID, groupFilter, groupDN) if source.GroupTeamMap != "" || source.GroupTeamMapRemoval {
groupSearch := ldap.NewSearchRequest( teamsToAdd, teamsToRemove = source.getMappedMemberships(usersLdapGroups, userAttributeListedInGroup)
groupDN, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, groupFilter,
[]string{source.GroupMemberUID},
nil)
srg, err := l.Search(groupSearch)
if err != nil {
log.Error("LDAP group search failed: %v", err)
return nil
} else if len(srg.Entries) < 1 {
log.Error("LDAP group search failed: 0 entries")
return nil
}
isMember := false
Entries:
for _, group := range srg.Entries {
for _, member := range group.GetAttributeValues(source.GroupMemberUID) {
if (source.UserUID == "dn" && member == sr.Entries[0].DN) || member == uid {
isMember = true
break Entries
}
}
}
if !isMember {
log.Error("LDAP group membership test failed")
return nil
} }
} }
if isAttributeSSHPublicKeySet { if isAttributeSSHPublicKeySet {
sshPublicKey = sr.Entries[0].GetAttributeValues(source.AttributeSSHPublicKey) sshPublicKey = sr.Entries[0].GetAttributeValues(source.AttributeSSHPublicKey)
} }
isAdmin := checkAdmin(l, source, userDN) isAdmin := checkAdmin(l, source, userDN)
var isRestricted bool var isRestricted bool
if !isAdmin { if !isAdmin {
isRestricted = checkRestricted(l, source, userDN) isRestricted = checkRestricted(l, source, userDN)
@ -436,12 +431,6 @@ func (source *Source) SearchEntry(name, passwd string, directBind bool) *SearchR
Avatar = sr.Entries[0].GetRawAttributeValue(source.AttributeAvatar) Avatar = sr.Entries[0].GetRawAttributeValue(source.AttributeAvatar)
} }
teamsToAdd := make(map[string][]string)
teamsToRemove := make(map[string][]string)
if source.GroupsEnabled && (source.GroupTeamMap != "" || source.GroupTeamMapRemoval) {
teamsToAdd, teamsToRemove = source.getMappedMemberships(l, uid)
}
if !directBind && source.AttributesInBind { if !directBind && source.AttributesInBind {
// binds user (checking password) after looking-up attributes in BindDN context // binds user (checking password) after looking-up attributes in BindDN context
err = bindUser(l, userDN, passwd) err = bindUser(l, userDN, passwd)
@ -520,19 +509,29 @@ func (source *Source) SearchEntries() ([]*SearchResult, error) {
return nil, err return nil, err
} }
result := make([]*SearchResult, len(sr.Entries)) result := make([]*SearchResult, 0, len(sr.Entries))
for i, v := range sr.Entries { for _, v := range sr.Entries {
teamsToAdd := make(map[string][]string) teamsToAdd := make(map[string][]string)
teamsToRemove := make(map[string][]string) teamsToRemove := make(map[string][]string)
if source.GroupsEnabled && (source.GroupTeamMap != "" || source.GroupTeamMapRemoval) {
userAttributeListedInGroup := v.GetAttributeValue(source.UserUID) if source.GroupsEnabled {
if source.UserUID == "dn" || source.UserUID == "DN" { userAttributeListedInGroup := source.getUserAttributeListedInGroup(v)
userAttributeListedInGroup = v.DN
if source.GroupFilter != "" {
usersLdapGroups := source.listLdapGroupMemberships(l, userAttributeListedInGroup, true)
if len(usersLdapGroups) == 0 {
continue
} }
teamsToAdd, teamsToRemove = source.getMappedMemberships(l, userAttributeListedInGroup)
} }
result[i] = &SearchResult{
if source.GroupTeamMap != "" || source.GroupTeamMapRemoval {
usersLdapGroups := source.listLdapGroupMemberships(l, userAttributeListedInGroup, false)
teamsToAdd, teamsToRemove = source.getMappedMemberships(usersLdapGroups, userAttributeListedInGroup)
}
}
user := &SearchResult{
Username: v.GetAttributeValue(source.AttributeUsername), Username: v.GetAttributeValue(source.AttributeUsername),
Name: v.GetAttributeValue(source.AttributeName), Name: v.GetAttributeValue(source.AttributeName),
Surname: v.GetAttributeValue(source.AttributeSurname), Surname: v.GetAttributeValue(source.AttributeSurname),
@ -541,16 +540,22 @@ func (source *Source) SearchEntries() ([]*SearchResult, error) {
LdapTeamAdd: teamsToAdd, LdapTeamAdd: teamsToAdd,
LdapTeamRemove: teamsToRemove, LdapTeamRemove: teamsToRemove,
} }
if !result[i].IsAdmin {
result[i].IsRestricted = checkRestricted(l, source, v.DN) if !user.IsAdmin {
user.IsRestricted = checkRestricted(l, source, v.DN)
} }
if isAttributeSSHPublicKeySet { if isAttributeSSHPublicKeySet {
result[i].SSHPublicKey = v.GetAttributeValues(source.AttributeSSHPublicKey) user.SSHPublicKey = v.GetAttributeValues(source.AttributeSSHPublicKey)
} }
if isAtributeAvatarSet { if isAtributeAvatarSet {
result[i].Avatar = v.GetRawAttributeValue(source.AttributeAvatar) user.Avatar = v.GetRawAttributeValue(source.AttributeAvatar)
} }
result[i].LowerName = strings.ToLower(result[i].Username)
user.LowerName = strings.ToLower(user.Username)
result = append(result, user)
} }
return result, nil return result, nil

View file

@ -11,12 +11,14 @@ import (
"testing" "testing"
"code.gitea.io/gitea/models" "code.gitea.io/gitea/models"
auth_model "code.gitea.io/gitea/models/auth"
"code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/organization"
"code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/translation" "code.gitea.io/gitea/modules/translation"
"code.gitea.io/gitea/services/auth" "code.gitea.io/gitea/services/auth"
"code.gitea.io/gitea/services/auth/source/ldap"
"code.gitea.io/gitea/tests" "code.gitea.io/gitea/tests"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
@ -102,13 +104,28 @@ func getLDAPServerHost() string {
return host return host
} }
func addAuthSourceLDAP(t *testing.T, sshKeyAttribute string, groupMapParams ...string) { func getLDAPServerPort() string {
port := os.Getenv("TEST_LDAP_PORT")
if len(port) == 0 {
port = "389"
}
return port
}
func addAuthSourceLDAP(t *testing.T, sshKeyAttribute, groupFilter string, groupMapParams ...string) {
groupTeamMapRemoval := "off" groupTeamMapRemoval := "off"
groupTeamMap := "" groupTeamMap := ""
if len(groupMapParams) == 2 { if len(groupMapParams) == 2 {
groupTeamMapRemoval = groupMapParams[0] groupTeamMapRemoval = groupMapParams[0]
groupTeamMap = groupMapParams[1] groupTeamMap = groupMapParams[1]
} }
// Modify user filter to test group filter explicitly
userFilter := "(&(objectClass=inetOrgPerson)(memberOf=cn=git,ou=people,dc=planetexpress,dc=com)(uid=%s))"
if groupFilter != "" {
userFilter = "(&(objectClass=inetOrgPerson)(uid=%s))"
}
session := loginUser(t, "user1") session := loginUser(t, "user1")
csrf := GetCSRF(t, session, "/admin/auths/new") csrf := GetCSRF(t, session, "/admin/auths/new")
req := NewRequestWithValues(t, "POST", "/admin/auths/new", map[string]string{ req := NewRequestWithValues(t, "POST", "/admin/auths/new", map[string]string{
@ -116,11 +133,11 @@ func addAuthSourceLDAP(t *testing.T, sshKeyAttribute string, groupMapParams ...s
"type": "2", "type": "2",
"name": "ldap", "name": "ldap",
"host": getLDAPServerHost(), "host": getLDAPServerHost(),
"port": "389", "port": getLDAPServerPort(),
"bind_dn": "uid=gitea,ou=service,dc=planetexpress,dc=com", "bind_dn": "uid=gitea,ou=service,dc=planetexpress,dc=com",
"bind_password": "password", "bind_password": "password",
"user_base": "ou=people,dc=planetexpress,dc=com", "user_base": "ou=people,dc=planetexpress,dc=com",
"filter": "(&(objectClass=inetOrgPerson)(memberOf=cn=git,ou=people,dc=planetexpress,dc=com)(uid=%s))", "filter": userFilter,
"admin_filter": "(memberOf=cn=admin_staff,ou=people,dc=planetexpress,dc=com)", "admin_filter": "(memberOf=cn=admin_staff,ou=people,dc=planetexpress,dc=com)",
"restricted_filter": "(uid=leela)", "restricted_filter": "(uid=leela)",
"attribute_username": "uid", "attribute_username": "uid",
@ -133,6 +150,7 @@ func addAuthSourceLDAP(t *testing.T, sshKeyAttribute string, groupMapParams ...s
"groups_enabled": "on", "groups_enabled": "on",
"group_dn": "ou=people,dc=planetexpress,dc=com", "group_dn": "ou=people,dc=planetexpress,dc=com",
"group_member_uid": "member", "group_member_uid": "member",
"group_filter": groupFilter,
"group_team_map": groupTeamMap, "group_team_map": groupTeamMap,
"group_team_map_removal": groupTeamMapRemoval, "group_team_map_removal": groupTeamMapRemoval,
"user_uid": "DN", "user_uid": "DN",
@ -146,7 +164,7 @@ func TestLDAPUserSignin(t *testing.T) {
return return
} }
defer tests.PrepareTestEnv(t)() defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "") addAuthSourceLDAP(t, "", "")
u := gitLDAPUsers[0] u := gitLDAPUsers[0]
@ -163,7 +181,7 @@ func TestLDAPUserSignin(t *testing.T) {
func TestLDAPAuthChange(t *testing.T) { func TestLDAPAuthChange(t *testing.T) {
defer tests.PrepareTestEnv(t)() defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "") addAuthSourceLDAP(t, "", "")
session := loginUser(t, "user1") session := loginUser(t, "user1")
req := NewRequest(t, "GET", "/admin/auths") req := NewRequest(t, "GET", "/admin/auths")
@ -221,7 +239,7 @@ func TestLDAPUserSync(t *testing.T) {
return return
} }
defer tests.PrepareTestEnv(t)() defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "") addAuthSourceLDAP(t, "", "")
auth.SyncExternalUsers(context.Background(), true) auth.SyncExternalUsers(context.Background(), true)
session := loginUser(t, "user1") session := loginUser(t, "user1")
@ -266,13 +284,72 @@ func TestLDAPUserSync(t *testing.T) {
} }
} }
func TestLDAPUserSyncWithGroupFilter(t *testing.T) {
if skipLDAPTests() {
t.Skip()
return
}
defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "", "(cn=git)")
// Assert a user not a member of the LDAP group "cn=git" cannot login
// This test may look like TestLDAPUserSigninFailed but it is not.
// The later test uses user filter containing group membership filter (memberOf)
// This test is for the case when LDAP user records may not be linked with
// all groups the user is a member of, the user filter is modified accordingly inside
// the addAuthSourceLDAP based on the value of the groupFilter
u := otherLDAPUsers[0]
testLoginFailed(t, u.UserName, u.Password, translation.NewLocale("en-US").Tr("form.username_password_incorrect"))
auth.SyncExternalUsers(context.Background(), true)
// Assert members of LDAP group "cn=git" are added
for _, gitLDAPUser := range gitLDAPUsers {
unittest.BeanExists(t, &user_model.User{
Name: gitLDAPUser.UserName,
})
}
// Assert everyone else is not added
for _, gitLDAPUser := range otherLDAPUsers {
unittest.AssertNotExistsBean(t, &user_model.User{
Name: gitLDAPUser.UserName,
})
}
ldapSource := unittest.AssertExistsAndLoadBean(t, &auth_model.Source{
Name: "ldap",
})
ldapConfig := ldapSource.Cfg.(*ldap.Source)
ldapConfig.GroupFilter = "(cn=ship_crew)"
auth_model.UpdateSource(ldapSource)
auth.SyncExternalUsers(context.Background(), true)
for _, gitLDAPUser := range gitLDAPUsers {
if gitLDAPUser.UserName == "fry" || gitLDAPUser.UserName == "leela" || gitLDAPUser.UserName == "bender" {
// Assert members of the LDAP group "cn-ship_crew" are still active
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{
Name: gitLDAPUser.UserName,
})
assert.True(t, user.IsActive, "User %s should be active", gitLDAPUser.UserName)
} else {
// Assert everyone else is inactive
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{
Name: gitLDAPUser.UserName,
})
assert.False(t, user.IsActive, "User %s should be inactive", gitLDAPUser.UserName)
}
}
}
func TestLDAPUserSigninFailed(t *testing.T) { func TestLDAPUserSigninFailed(t *testing.T) {
if skipLDAPTests() { if skipLDAPTests() {
t.Skip() t.Skip()
return return
} }
defer tests.PrepareTestEnv(t)() defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "") addAuthSourceLDAP(t, "", "")
u := otherLDAPUsers[0] u := otherLDAPUsers[0]
testLoginFailed(t, u.UserName, u.Password, translation.NewLocale("en-US").Tr("form.username_password_incorrect")) testLoginFailed(t, u.UserName, u.Password, translation.NewLocale("en-US").Tr("form.username_password_incorrect"))
@ -284,7 +361,7 @@ func TestLDAPUserSSHKeySync(t *testing.T) {
return return
} }
defer tests.PrepareTestEnv(t)() defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "sshPublicKey") addAuthSourceLDAP(t, "sshPublicKey", "")
auth.SyncExternalUsers(context.Background(), true) auth.SyncExternalUsers(context.Background(), true)
@ -317,7 +394,7 @@ func TestLDAPGroupTeamSyncAddMember(t *testing.T) {
return return
} }
defer tests.PrepareTestEnv(t)() defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "", "on", `{"cn=ship_crew,ou=people,dc=planetexpress,dc=com":{"org26": ["team11"]},"cn=admin_staff,ou=people,dc=planetexpress,dc=com": {"non-existent": ["non-existent"]}}`) addAuthSourceLDAP(t, "", "", "on", `{"cn=ship_crew,ou=people,dc=planetexpress,dc=com":{"org26": ["team11"]},"cn=admin_staff,ou=people,dc=planetexpress,dc=com": {"non-existent": ["non-existent"]}}`)
org, err := organization.GetOrgByName("org26") org, err := organization.GetOrgByName("org26")
assert.NoError(t, err) assert.NoError(t, err)
team, err := organization.GetTeam(db.DefaultContext, org.ID, "team11") team, err := organization.GetTeam(db.DefaultContext, org.ID, "team11")
@ -362,7 +439,7 @@ func TestLDAPGroupTeamSyncRemoveMember(t *testing.T) {
return return
} }
defer tests.PrepareTestEnv(t)() defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "", "on", `{"cn=dispatch,ou=people,dc=planetexpress,dc=com": {"org26": ["team11"]}}`) addAuthSourceLDAP(t, "", "", "on", `{"cn=dispatch,ou=people,dc=planetexpress,dc=com": {"org26": ["team11"]}}`)
org, err := organization.GetOrgByName("org26") org, err := organization.GetOrgByName("org26")
assert.NoError(t, err) assert.NoError(t, err)
team, err := organization.GetTeam(db.DefaultContext, org.ID, "team11") team, err := organization.GetTeam(db.DefaultContext, org.ID, "team11")
@ -398,7 +475,7 @@ func TestBrokenLDAPMapUserSignin(t *testing.T) {
return return
} }
defer tests.PrepareTestEnv(t)() defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "", "on", `{"NOT_A_VALID_JSON"["MISSING_DOUBLE_POINT"]}`) addAuthSourceLDAP(t, "", "", "on", `{"NOT_A_VALID_JSON"["MISSING_DOUBLE_POINT"]}`)
u := gitLDAPUsers[0] u := gitLDAPUsers[0]