From 7b8c86b38ee69a68d279f720e996b1bc547175f4 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 13 Apr 2021 08:38:04 -0700 Subject: [PATCH] Handle error cases during LDAP user search and bind --- internal/upstreamldap/upstreamldap.go | 9 - internal/upstreamldap/upstreamldap_test.go | 227 +++++++++++++++++++++ 2 files changed, 227 insertions(+), 9 deletions(-) diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index eca83707..b5d79c80 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -188,7 +188,6 @@ func (p *Provider) AuthenticateUser(ctx context.Context, username, password stri err = conn.Bind(p.BindUsername, p.BindPassword) if err != nil { - // TODO test this return nil, false, fmt.Errorf(`error binding as "%s" before user search: %w`, p.BindUsername, err) } @@ -210,37 +209,31 @@ func (p *Provider) AuthenticateUser(ctx context.Context, username, password stri func (p *Provider) searchAndBindUser(conn Conn, username string, password string) (string, string, error) { searchResult, err := conn.Search(p.userSearchRequest(username)) if err != nil { - // TODO test this return "", "", fmt.Errorf(`error searching for user "%s": %w`, username, err) } if len(searchResult.Entries) != 1 { - // TODO test this return "", "", fmt.Errorf(`searching for user "%s" resulted in %d search results, but expected 1 result`, username, len(searchResult.Entries), ) } userEntry := searchResult.Entries[0] if len(userEntry.DN) == 0 { - // TODO test this return "", "", fmt.Errorf(`searching for user "%s" resulted in search result without DN`, username) } mappedUsername, err := p.getSearchResultAttributeValue(p.UserSearch.UsernameAttribute, userEntry, username) if err != nil { - // TODO test this return "", "", err } mappedUID, err := p.getSearchResultAttributeValue(p.UserSearch.UIDAttribute, userEntry, username) if err != nil { - // TODO test this return "", "", err } // Take care that any other LDAP commands after this bind will be run as this user instead of as the configured BindUsername! err = conn.Bind(userEntry.DN, password) if err != nil { - // TODO test this return "", "", fmt.Errorf(`error binding for user "%s" using provided password against DN "%s": %w`, username, userEntry.DN, err) } @@ -294,7 +287,6 @@ func (p *Provider) getSearchResultAttributeValue(attributeName string, fromUserE attributeValues := fromUserEntry.GetAttributeValues(attributeName) if len(attributeValues) != 1 { - // TODO test this return "", fmt.Errorf(`found %d values for attribute "%s" while searching for user "%s", but expected 1 result`, len(attributeValues), attributeName, username, ) @@ -302,7 +294,6 @@ func (p *Provider) getSearchResultAttributeValue(attributeName string, fromUserE attributeValue := attributeValues[0] if len(attributeValue) == 0 { - // TODO test this return "", fmt.Errorf(`found empty value for attribute "%s" while searching for user "%s", but expected value to be non-empty`, attributeName, username, ) diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 9b17c537..61b2a305 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -255,6 +255,233 @@ func TestAuthenticateUser(t *testing.T) { dialError: errors.New("some dial error"), wantError: fmt.Sprintf(`error dialing host "%s": some dial error`, testHost), }, + { + name: "when binding as the bind user returns an error", + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Return(errors.New("some bind error")).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`error binding as "%s" before user search: some bind error`, testBindUsername), + }, + { + name: "when searching for the user returns an error", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(nil, errors.New("some search error")).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`error searching for user "%s": some search error`, testUpstreamUsername), + }, + { + name: "when searching for the user returns no results", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{}, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`searching for user "%s" resulted in 0 search results, but expected 1 result`, testUpstreamUsername), + }, + { + name: "when searching for the user returns multiple results", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + {DN: testSearchResultDNValue}, + {DN: "some-other-dn"}, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`searching for user "%s" resulted in 2 search results, but expected 1 result`, testUpstreamUsername), + }, + { + name: "when searching for the user returns a user without a DN", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + {DN: ""}, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`searching for user "%s" resulted in search result without DN`, testUpstreamUsername), + }, + { + name: "when searching for the user returns a user without an expected username attribute", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + ldap.NewEntryAttribute(testUserSearchUIDAttribute, []string{testSearchResultUIDAttributeValue}), + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`found 0 values for attribute "%s" while searching for user "%s", but expected 1 result`, testUserSearchUsernameAttribute, testUpstreamUsername), + }, + { + name: "when searching for the user returns a user with too many values for the expected username attribute", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + ldap.NewEntryAttribute(testUserSearchUsernameAttribute, []string{ + testSearchResultUsernameAttributeValue, + "unexpected-additional-value", + }), + ldap.NewEntryAttribute(testUserSearchUIDAttribute, []string{testSearchResultUIDAttributeValue}), + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`found 2 values for attribute "%s" while searching for user "%s", but expected 1 result`, testUserSearchUsernameAttribute, testUpstreamUsername), + }, + { + name: "when searching for the user returns a user with an empty value for the expected username attribute", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + ldap.NewEntryAttribute(testUserSearchUsernameAttribute, []string{""}), + ldap.NewEntryAttribute(testUserSearchUIDAttribute, []string{testSearchResultUIDAttributeValue}), + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`found empty value for attribute "%s" while searching for user "%s", but expected value to be non-empty`, testUserSearchUsernameAttribute, testUpstreamUsername), + }, + { + name: "when searching for the user returns a user without an expected UID attribute", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + ldap.NewEntryAttribute(testUserSearchUsernameAttribute, []string{testSearchResultUsernameAttributeValue}), + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`found 0 values for attribute "%s" while searching for user "%s", but expected 1 result`, testUserSearchUIDAttribute, testUpstreamUsername), + }, + { + name: "when searching for the user returns a user with too many values for the expected UID attribute", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + ldap.NewEntryAttribute(testUserSearchUsernameAttribute, []string{testSearchResultUsernameAttributeValue}), + ldap.NewEntryAttribute(testUserSearchUIDAttribute, []string{ + testSearchResultUIDAttributeValue, + "unexpected-additional-value", + }), + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`found 2 values for attribute "%s" while searching for user "%s", but expected 1 result`, testUserSearchUIDAttribute, testUpstreamUsername), + }, + { + name: "when searching for the user returns a user with an empty value for the expected UID attribute", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + ldap.NewEntryAttribute(testUserSearchUsernameAttribute, []string{testSearchResultUsernameAttributeValue}), + ldap.NewEntryAttribute(testUserSearchUIDAttribute, []string{""}), + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`found empty value for attribute "%s" while searching for user "%s", but expected value to be non-empty`, testUserSearchUIDAttribute, testUpstreamUsername), + }, + { + name: "when binding as the found user returns an error", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + ldap.NewEntryAttribute(testUserSearchUsernameAttribute, []string{testSearchResultUsernameAttributeValue}), + ldap.NewEntryAttribute(testUserSearchUIDAttribute, []string{testSearchResultUIDAttributeValue}), + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Bind(testSearchResultDNValue, testUpstreamPassword).Return(errors.New("some bind error")).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: fmt.Sprintf(`error binding for user "%s" using provided password against DN "%s": some bind error`, testUpstreamUsername, testSearchResultDNValue), + }, } for _, test := range tests {