From c427b8f920a17bbd4b3efffaeec35109088e6268 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Sun, 15 May 2022 16:37:23 +1000 Subject: [PATCH] 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. --- internal/authentication/ldap_user_provider.go | 21 +- .../ldap_user_provider_startup.go | 15 +- .../authentication/ldap_user_provider_test.go | 218 ++++++++++++++++++ 3 files changed, 243 insertions(+), 11 deletions(-) diff --git a/internal/authentication/ldap_user_provider.go b/internal/authentication/ldap_user_provider.go index 23d5518e5..6142df7d9 100644 --- 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) + 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 == "" { diff --git a/internal/authentication/ldap_user_provider_startup.go b/internal/authentication/ldap_user_provider_startup.go index 71c881297..4c87c9013 100644 --- 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 != "" { diff --git a/internal/authentication/ldap_user_provider_test.go b/internal/authentication/ldap_user_provider_test.go index d158fd607..a63cb1091 100644 --- 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().