fix(authentication): err when user/display name same ldap attribute (#3364)

This fixes an issue when both the username and display name attributes are the same. If the username attribute is the same as the display name attribute previously we only set the display name profile value which is incorrect. We should set the username profile value instead and allow the display name to be blank.
2022-05-15 16:37:23 +10:00 · 2022-05-15 16:37:23 +10:00 · c427b8f920
parent 0c28788c5f
commit c427b8f920
3 changed files with 243 additions and 11 deletions
--- a/internal/authentication/ldap_user_provider.go
+++ b/internal/authentication/ldap_user_provider.go
@ -339,14 +339,9 @@ func (p *LDAPUserProvider) getUserProfile(client LDAPClient, username string) (p
 	}

 	for _, attr := range searchResult.Entries[0].Attributes {
-		switch attr.Name {
-		case p.config.DisplayNameAttribute:
-			userProfile.DisplayName = attr.Values[0]
-		case p.config.MailAttribute:
-			userProfile.Emails = attr.Values
-		case p.config.UsernameAttribute:
 		attrs := len(attr.Values)

+		if attr.Name == p.config.UsernameAttribute {
 			switch attrs {
 			case 1:
 				userProfile.Username = attr.Values[0]
@ -358,6 +353,18 @@ func (p *LDAPUserProvider) getUserProfile(client LDAPClient, username string) (p
 					username, attrs, p.config.UsernameAttribute)
 			}
 		}
+
+		if attrs == 0 {
+			continue
+		}
+
+		if attr.Name == p.config.MailAttribute {
+			userProfile.Emails = attr.Values
+		}
+
+		if attr.Name == p.config.DisplayNameAttribute {
+			userProfile.DisplayName = attr.Values[0]
+		}
 	}

 	if userProfile.Username == "" {
--- a/internal/authentication/ldap_user_provider_startup.go
+++ b/internal/authentication/ldap_user_provider_startup.go
@ -6,6 +6,7 @@ import (
 	"github.com/go-ldap/ldap/v3"

 	"github.com/authelia/authelia/v4/internal/configuration/schema"
+	"github.com/authelia/authelia/v4/internal/utils"
 )

 // StartupCheck implements the startup check provider interface.
@ -88,10 +89,16 @@ func (p *LDAPUserProvider) parseDynamicUsersConfiguration() {

 	p.log.Tracef("Dynamically generated users filter is %s", p.config.UsersFilter)

-	p.usersAttributes = []string{
-		p.config.DisplayNameAttribute,
-		p.config.MailAttribute,
-		p.config.UsernameAttribute,
+	if !utils.IsStringInSlice(p.config.UsernameAttribute, p.usersAttributes) {
+		p.usersAttributes = append(p.usersAttributes, p.config.UsernameAttribute)
+	}
+
+	if !utils.IsStringInSlice(p.config.MailAttribute, p.usersAttributes) {
+		p.usersAttributes = append(p.usersAttributes, p.config.MailAttribute)
+	}
+
+	if !utils.IsStringInSlice(p.config.DisplayNameAttribute, p.usersAttributes) {
+		p.usersAttributes = append(p.usersAttributes, p.config.DisplayNameAttribute)
 	}

 	if p.config.AdditionalUsersDN != "" {
--- a/internal/authentication/ldap_user_provider_test.go
+++ b/internal/authentication/ldap_user_provider_test.go
@ -545,6 +545,222 @@ func TestShouldEscapeUserInput(t *testing.T) {
 	assert.EqualError(t, err, "user not found")
 }

+func TestShouldReturnEmailWhenAttributeSameAsUsername(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockFactory := NewMockLDAPClientFactory(ctrl)
+	mockClient := NewMockLDAPClient(ctrl)
+
+	ldapClient := newLDAPUserProvider(
+		schema.LDAPAuthenticationBackendConfiguration{
+			URL:                  "ldap://127.0.0.1:389",
+			User:                 "cn=admin,dc=example,dc=com",
+			Password:             "password",
+			UsernameAttribute:    "mail",
+			MailAttribute:        "mail",
+			DisplayNameAttribute: "displayName",
+			UsersFilter:          "(&({username_attribute}={input})(objectClass=inetOrgPerson))",
+			AdditionalUsersDN:    "ou=users",
+			BaseDN:               "dc=example,dc=com",
+		},
+		false,
+		nil,
+		mockFactory)
+
+	assert.Equal(t, []string{"mail", "displayName"}, ldapClient.usersAttributes)
+
+	dialURL := mockFactory.EXPECT().
+		DialURL(gomock.Eq("ldap://127.0.0.1:389"), gomock.Any()).
+		Return(mockClient, nil)
+
+	bind := mockClient.EXPECT().
+		Bind(gomock.Eq("cn=admin,dc=example,dc=com"), gomock.Eq("password")).
+		Return(nil)
+
+	search := mockClient.EXPECT().
+		Search(NewSearchRequestMatcher("(&(mail=john@example.com)(objectClass=inetOrgPerson))")).
+		Return(&ldap.SearchResult{
+			Entries: []*ldap.Entry{
+				{
+					DN: "uid=john,dc=example,dc=com",
+					Attributes: []*ldap.EntryAttribute{
+						{
+							Name:   "mail",
+							Values: []string{"john@example.com"},
+						},
+						{
+							Name:   "displayName",
+							Values: []string{"John Doe"},
+						},
+					},
+				},
+			},
+		}, nil)
+
+	gomock.InOrder(dialURL, bind, search)
+
+	client, err := ldapClient.connect()
+	assert.NoError(t, err)
+
+	profile, err := ldapClient.getUserProfile(client, "john@example.com")
+
+	assert.NoError(t, err)
+	require.NotNil(t, profile)
+
+	assert.Equal(t, "uid=john,dc=example,dc=com", profile.DN)
+	assert.Equal(t, "john@example.com", profile.Username)
+	assert.Equal(t, "John Doe", profile.DisplayName)
+
+	require.Len(t, profile.Emails, 1)
+	assert.Equal(t, "john@example.com", profile.Emails[0])
+}
+
+func TestShouldReturnUsernameAndBlankDisplayNameWhenAttributesTheSame(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockFactory := NewMockLDAPClientFactory(ctrl)
+	mockClient := NewMockLDAPClient(ctrl)
+
+	ldapClient := newLDAPUserProvider(
+		schema.LDAPAuthenticationBackendConfiguration{
+			URL:                  "ldap://127.0.0.1:389",
+			User:                 "cn=admin,dc=example,dc=com",
+			Password:             "password",
+			UsernameAttribute:    "uid",
+			MailAttribute:        "mail",
+			DisplayNameAttribute: "uid",
+			UsersFilter:          "(&(|({username_attribute}={input})({mail_attribute}={input}))(objectClass=inetOrgPerson))",
+			AdditionalUsersDN:    "ou=users",
+			BaseDN:               "dc=example,dc=com",
+		},
+		false,
+		nil,
+		mockFactory)
+
+	assert.Equal(t, []string{"uid", "mail"}, ldapClient.usersAttributes)
+
+	dialURL := mockFactory.EXPECT().
+		DialURL(gomock.Eq("ldap://127.0.0.1:389"), gomock.Any()).
+		Return(mockClient, nil)
+
+	bind := mockClient.EXPECT().
+		Bind(gomock.Eq("cn=admin,dc=example,dc=com"), gomock.Eq("password")).
+		Return(nil)
+
+	search := mockClient.EXPECT().
+		Search(NewSearchRequestMatcher("(&(|(uid=john@example.com)(mail=john@example.com))(objectClass=inetOrgPerson))")).
+		Return(&ldap.SearchResult{
+			Entries: []*ldap.Entry{
+				{
+					DN: "uid=john,dc=example,dc=com",
+					Attributes: []*ldap.EntryAttribute{
+						{
+							Name:   "uid",
+							Values: []string{"john"},
+						},
+						{
+							Name:   "mail",
+							Values: []string{"john@example.com"},
+						},
+					},
+				},
+			},
+		}, nil)
+
+	gomock.InOrder(dialURL, bind, search)
+
+	client, err := ldapClient.connect()
+	assert.NoError(t, err)
+
+	profile, err := ldapClient.getUserProfile(client, "john@example.com")
+
+	assert.NoError(t, err)
+	require.NotNil(t, profile)
+
+	assert.Equal(t, "uid=john,dc=example,dc=com", profile.DN)
+	assert.Equal(t, "john", profile.Username)
+	assert.Equal(t, "john", profile.DisplayName)
+
+	require.Len(t, profile.Emails, 1)
+	assert.Equal(t, "john@example.com", profile.Emails[0])
+}
+
+func TestShouldReturnBlankEmailAndDisplayNameWhenAttrsLenZero(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockFactory := NewMockLDAPClientFactory(ctrl)
+	mockClient := NewMockLDAPClient(ctrl)
+
+	ldapClient := newLDAPUserProvider(
+		schema.LDAPAuthenticationBackendConfiguration{
+			URL:                  "ldap://127.0.0.1:389",
+			User:                 "cn=admin,dc=example,dc=com",
+			Password:             "password",
+			UsernameAttribute:    "uid",
+			MailAttribute:        "mail",
+			DisplayNameAttribute: "displayName",
+			UsersFilter:          "(&(|({username_attribute}={input})({mail_attribute}={input}))(objectClass=inetOrgPerson))",
+			AdditionalUsersDN:    "ou=users",
+			BaseDN:               "dc=example,dc=com",
+		},
+		false,
+		nil,
+		mockFactory)
+
+	assert.Equal(t, []string{"uid", "mail", "displayName"}, ldapClient.usersAttributes)
+
+	dialURL := mockFactory.EXPECT().
+		DialURL(gomock.Eq("ldap://127.0.0.1:389"), gomock.Any()).
+		Return(mockClient, nil)
+
+	bind := mockClient.EXPECT().
+		Bind(gomock.Eq("cn=admin,dc=example,dc=com"), gomock.Eq("password")).
+		Return(nil)
+
+	search := mockClient.EXPECT().
+		Search(NewSearchRequestMatcher("(&(|(uid=john@example.com)(mail=john@example.com))(objectClass=inetOrgPerson))")).
+		Return(&ldap.SearchResult{
+			Entries: []*ldap.Entry{
+				{
+					DN: "uid=john,dc=example,dc=com",
+					Attributes: []*ldap.EntryAttribute{
+						{
+							Name:   "uid",
+							Values: []string{"john"},
+						},
+						{
+							Name:   "mail",
+							Values: []string{},
+						},
+						{
+							Name:   "displayName",
+							Values: []string{},
+						},
+					},
+				},
+			},
+		}, nil)
+
+	gomock.InOrder(dialURL, bind, search)
+
+	client, err := ldapClient.connect()
+	assert.NoError(t, err)
+
+	profile, err := ldapClient.getUserProfile(client, "john@example.com")
+
+	assert.NoError(t, err)
+	require.NotNil(t, profile)
+
+	assert.Equal(t, "uid=john,dc=example,dc=com", profile.DN)
+	assert.Equal(t, "john", profile.Username)
+	assert.Equal(t, "", profile.DisplayName)
+
+	assert.Len(t, profile.Emails, 0)
+}
+
 func TestShouldCombineUsernameFilterAndUsersFilter(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
@ -569,6 +785,8 @@ func TestShouldCombineUsernameFilterAndUsersFilter(t *testing.T) {
 		nil,
 		mockFactory)

+	assert.Equal(t, []string{"uid", "mail", "displayName"}, ldapClient.usersAttributes)
+
 	assert.True(t, ldapClient.usersFilterReplacementInput)

 	mockClient.EXPECT().