Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix group filter for ldap source sync #22506

Merged
merged 3 commits into from
Feb 2, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix group filter for ldap source sync
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
pezhovski committed Jan 30, 2023
commit 84db9f7d528af14e927ba38e607a5f5a91ecc453
137 changes: 71 additions & 66 deletions services/auth/source/ldap/source_search.go
Original file line number Diff line number Diff line change
@@ -196,22 +196,39 @@ func checkRestricted(l *ldap.Conn, ls *Source, userDN string) bool {
}

// 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
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(
source.GroupDN,
groupDN,
ldap.ScopeWholeSubtree,
ldap.NeverDerefAliases,
0,
0,
false,
groupFilter,
searchFilter,
[]string{},
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
}

@@ -238,9 +255,7 @@ func (source *Source) mapLdapGroupsToTeams() map[string]map[string][]string {
}

// 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) {
// get all LDAP group memberships for user
usersLdapGroups := source.listLdapGroupMemberships(l, uid)
func (source *Source) getMappedMemberships(usersLdapGroups []string, uid string) (map[string][]string, map[string][]string) {
// unmarshall LDAP group team map from configs
ldapGroupsToTeams := source.mapLdapGroupsToTeams()
membershipsToAdd := map[string][]string{}
@@ -260,6 +275,14 @@ func (source *Source) getMappedMemberships(l *ldap.Conn, uid string) (map[string
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
func (source *Source) SearchEntry(name, passwd string, directBind bool) *SearchResult {
// 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)
surname := sr.Entries[0].GetAttributeValue(source.AttributeSurname)
mail := sr.Entries[0].GetAttributeValue(source.AttributeMail)
uid := sr.Entries[0].GetAttributeValue(source.UserUID)
if source.UserUID == "dn" || source.UserUID == "DN" {
uid = sr.Entries[0].DN
}

// Check group membership
if source.GroupsEnabled && source.GroupFilter != "" {
groupFilter, ok := source.sanitizedGroupFilter(source.GroupFilter)
if !ok {
return nil
}
groupDN, ok := source.sanitizedGroupDN(source.GroupDN)
if !ok {
return nil
}
teamsToAdd := make(map[string][]string)
teamsToRemove := make(map[string][]string)

log.Trace("Fetching groups '%v' with filter '%s' and base '%s'", source.GroupMemberUID, groupFilter, groupDN)
groupSearch := ldap.NewSearchRequest(
groupDN, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, groupFilter,
[]string{source.GroupMemberUID},
nil)
// Check group membership
if source.GroupsEnabled {
userAttributeListedInGroup := source.getUserAttributeListedInGroup(sr.Entries[0])
usersLdapGroups := source.listLdapGroupMemberships(l, userAttributeListedInGroup, true)

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")
if source.GroupFilter != "" && len(usersLdapGroups) == 0 {
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 source.GroupTeamMap != "" || source.GroupTeamMapRemoval {
teamsToAdd, teamsToRemove = source.getMappedMemberships(usersLdapGroups, userAttributeListedInGroup)
}
}

if isAttributeSSHPublicKeySet {
sshPublicKey = sr.Entries[0].GetAttributeValues(source.AttributeSSHPublicKey)
}

isAdmin := checkAdmin(l, source, userDN)

var isRestricted bool
if !isAdmin {
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)
}

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 {
// binds user (checking password) after looking-up attributes in BindDN context
err = bindUser(l, userDN, passwd)
@@ -520,19 +509,29 @@ func (source *Source) SearchEntries() ([]*SearchResult, error) {
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)
teamsToRemove := make(map[string][]string)
if source.GroupsEnabled && (source.GroupTeamMap != "" || source.GroupTeamMapRemoval) {
userAttributeListedInGroup := v.GetAttributeValue(source.UserUID)
if source.UserUID == "dn" || source.UserUID == "DN" {
userAttributeListedInGroup = v.DN

if source.GroupsEnabled {
userAttributeListedInGroup := source.getUserAttributeListedInGroup(v)

if source.GroupFilter != "" {
usersLdapGroups := source.listLdapGroupMemberships(l, userAttributeListedInGroup, true)
if len(usersLdapGroups) == 0 {
continue
}
}

if source.GroupTeamMap != "" || source.GroupTeamMapRemoval {
usersLdapGroups := source.listLdapGroupMemberships(l, userAttributeListedInGroup, false)
teamsToAdd, teamsToRemove = source.getMappedMemberships(usersLdapGroups, userAttributeListedInGroup)
}
teamsToAdd, teamsToRemove = source.getMappedMemberships(l, userAttributeListedInGroup)
}
result[i] = &SearchResult{

user := &SearchResult{
Username: v.GetAttributeValue(source.AttributeUsername),
Name: v.GetAttributeValue(source.AttributeName),
Surname: v.GetAttributeValue(source.AttributeSurname),
@@ -541,16 +540,22 @@ func (source *Source) SearchEntries() ([]*SearchResult, error) {
LdapTeamAdd: teamsToAdd,
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 {
result[i].SSHPublicKey = v.GetAttributeValues(source.AttributeSSHPublicKey)
user.SSHPublicKey = v.GetAttributeValues(source.AttributeSSHPublicKey)
}

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
99 changes: 88 additions & 11 deletions tests/integration/auth_ldap_test.go
Original file line number Diff line number Diff line change
@@ -11,12 +11,14 @@ import (
"testing"

"code.gitea.io/gitea/models"
auth_model "code.gitea.io/gitea/models/auth"
"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/organization"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/translation"
"code.gitea.io/gitea/services/auth"
"code.gitea.io/gitea/services/auth/source/ldap"
"code.gitea.io/gitea/tests"

"github.com/stretchr/testify/assert"
@@ -102,25 +104,40 @@ func getLDAPServerHost() string {
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"
groupTeamMap := ""
if len(groupMapParams) == 2 {
groupTeamMapRemoval = groupMapParams[0]
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")
csrf := GetCSRF(t, session, "/admin/auths/new")
req := NewRequestWithValues(t, "POST", "/admin/auths/new", map[string]string{
"_csrf": csrf,
"type": "2",
"name": "ldap",
"host": getLDAPServerHost(),
"port": "389",
"port": getLDAPServerPort(),
"bind_dn": "uid=gitea,ou=service,dc=planetexpress,dc=com",
"bind_password": "password",
"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)",
"restricted_filter": "(uid=leela)",
"attribute_username": "uid",
@@ -133,6 +150,7 @@ func addAuthSourceLDAP(t *testing.T, sshKeyAttribute string, groupMapParams ...s
"groups_enabled": "on",
"group_dn": "ou=people,dc=planetexpress,dc=com",
"group_member_uid": "member",
"group_filter": groupFilter,
"group_team_map": groupTeamMap,
"group_team_map_removal": groupTeamMapRemoval,
"user_uid": "DN",
@@ -146,7 +164,7 @@ func TestLDAPUserSignin(t *testing.T) {
return
}
defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "")
addAuthSourceLDAP(t, "", "")

u := gitLDAPUsers[0]

@@ -163,7 +181,7 @@ func TestLDAPUserSignin(t *testing.T) {

func TestLDAPAuthChange(t *testing.T) {
defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "")
addAuthSourceLDAP(t, "", "")

session := loginUser(t, "user1")
req := NewRequest(t, "GET", "/admin/auths")
@@ -221,7 +239,7 @@ func TestLDAPUserSync(t *testing.T) {
return
}
defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "")
addAuthSourceLDAP(t, "", "")
auth.SyncExternalUsers(context.Background(), true)

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) {
if skipLDAPTests() {
t.Skip()
return
}
defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "")
addAuthSourceLDAP(t, "", "")

u := otherLDAPUsers[0]
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
}
defer tests.PrepareTestEnv(t)()
addAuthSourceLDAP(t, "sshPublicKey")
addAuthSourceLDAP(t, "sshPublicKey", "")

auth.SyncExternalUsers(context.Background(), true)

@@ -317,7 +394,7 @@ func TestLDAPGroupTeamSyncAddMember(t *testing.T) {
return
}
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")
assert.NoError(t, err)
team, err := organization.GetTeam(db.DefaultContext, org.ID, "team11")
@@ -362,7 +439,7 @@ func TestLDAPGroupTeamSyncRemoveMember(t *testing.T) {
return
}
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")
assert.NoError(t, err)
team, err := organization.GetTeam(db.DefaultContext, org.ID, "team11")
@@ -398,7 +475,7 @@ func TestBrokenLDAPMapUserSignin(t *testing.T) {
return
}
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]