Skip to content

Commit 9047b8b

Browse files
helenfufuhelenfufu
authored
ldap getUserDN handle multiple users (#151)
* ensure one result entry in getUserDN * add failed-with-anon-bind-upn-domain-multiple-users-returned test case to TestClient_Authenticate * add unit tests for getUserDN * add unit tests for getUserBindDN * use single test directory with duplicated user instead of multiple test directories
1 parent 5c3419e commit 9047b8b

File tree

3 files changed

+339
-2
lines changed

3 files changed

+339
-2
lines changed

‎ldap/client.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -736,9 +736,10 @@ func (c *Client) getUserDN(bindDN, username string) (string, error) {
736736
if err != nil {
737737
return userDN, fmt.Errorf("%s: LDAP search failed for detecting user (baseDN: %q / filter: %q): %w", op, c.conf.UserDN, filter, err)
738738
}
739-
for _, e := range result.Entries {
740-
userDN = e.DN
739+
if len(result.Entries) != 1 {
740+
return "", fmt.Errorf("%s: LDAP search for user 0 or not unique", op)
741741
}
742+
userDN = result.Entries[0].DN
742743
} else {
743744
userDN = bindDN
744745
}

‎ldap/client_exported_test.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,15 @@ func TestClient_Authenticate(t *testing.T) {
4747
testdirectory.WithDefaults(t, &testdirectory.Defaults{UPNDomain: "example.com"}),
4848
testdirectory.WithMembersOf(t, "admin"))...,
4949
)
50+
// Set up a duplicated user to test the case where the search returns multiple users
51+
users = append(
52+
users,
53+
testdirectory.NewUsers(
54+
t,
55+
[]string{"mallory", "mallory"},
56+
testdirectory.WithDefaults(t, &testdirectory.Defaults{UPNDomain: "example.com"}),
57+
)...,
58+
)
5059
// add some attributes that we always want to filter out of an AuthResult,
5160
// so if we ever start seeing tests fail because of them; we know that we've
5261
// messed up the default filtering
@@ -59,6 +68,7 @@ func TestClient_Authenticate(t *testing.T) {
5968
td.SetUsers(users...)
6069
td.SetGroups(groups...)
6170
td.SetTokenGroups(tokenGroups)
71+
6272
tests := []struct {
6373
name string
6474
username string
@@ -509,6 +519,22 @@ func TestClient_Authenticate(t *testing.T) {
509519
opts: []ldap.Option{ldap.WithGroups(), ldap.WithEmptyAnonymousGroupSearch()},
510520
wantGroups: []string{groups[0].DN},
511521
},
522+
{
523+
name: "failed-with-anon-bind-upn-domain-multiple-users-returned",
524+
username: "mallory",
525+
password: "password",
526+
clientConfig: &ldap.ClientConfig{
527+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
528+
Certificates: []string{td.Cert()},
529+
DiscoverDN: true,
530+
UserDN: testdirectory.DefaultUserDN,
531+
GroupDN: testdirectory.DefaultGroupDN,
532+
UPNDomain: "example.com",
533+
},
534+
opts: []ldap.Option{ldap.WithGroups()},
535+
wantErr: true,
536+
wantErrContains: "LDAP search for binddn 0 or not unique",
537+
},
512538
}
513539
for _, tc := range tests {
514540
t.Run(tc.name, func(t *testing.T) {

‎ldap/client_test.go‎

Lines changed: 310 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,316 @@ func TestClient_connect(t *testing.T) {
456456
})
457457
}
458458

459+
func TestClient_getUserBindDN(t *testing.T) {
460+
t.Parallel()
461+
testCtx := context.Background()
462+
logger := hclog.New(&hclog.LoggerOptions{
463+
Name: "test-logger",
464+
Level: hclog.Error,
465+
})
466+
td := testdirectory.Start(nil,
467+
testdirectory.WithDefaults(nil, &testdirectory.Defaults{AllowAnonymousBind: true}),
468+
testdirectory.WithLogger(t, logger),
469+
)
470+
users := testdirectory.NewUsers(t, []string{"alice", "bob"})
471+
users = append(
472+
users,
473+
testdirectory.NewUsers(
474+
t,
475+
[]string{"eve"},
476+
testdirectory.WithDefaults(t, &testdirectory.Defaults{UPNDomain: "example.com"}))...,
477+
)
478+
// Set up a duplicated user to test the case where the search returns multiple users
479+
users = append(
480+
users,
481+
testdirectory.NewUsers(
482+
t,
483+
[]string{"mallory", "mallory"},
484+
)...,
485+
)
486+
td.SetUsers(users...)
487+
488+
cases := map[string]struct {
489+
conf *ClientConfig
490+
username string
491+
492+
want string
493+
wantErr bool
494+
wantErrIs error
495+
wantErrContains string
496+
}{
497+
"fail: missing username": {
498+
conf: &ClientConfig{
499+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
500+
Certificates: []string{td.Cert()},
501+
},
502+
username: "",
503+
504+
wantErr: true,
505+
wantErrIs: ErrInvalidParameter,
506+
wantErrContains: "missing username",
507+
},
508+
"fail: missing all of discoverdn, binddn, bindpass, upndomain, userdn": {
509+
conf: &ClientConfig{
510+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
511+
Certificates: []string{td.Cert()},
512+
},
513+
username: "alice",
514+
515+
wantErr: true,
516+
wantErrIs: ErrInvalidParameter,
517+
wantErrContains: "cannot derive UserBindDN based on config (see combination of: discoverdn, binddn, bindpass, upndomain, userdn)",
518+
},
519+
"fail: search fails to find user": {
520+
conf: &ClientConfig{
521+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
522+
Certificates: []string{td.Cert()},
523+
DiscoverDN: true,
524+
},
525+
username: "nonexistent",
526+
527+
wantErr: true,
528+
wantErrContains: "LDAP search for binddn failed",
529+
},
530+
"fail: search returns multiple users": {
531+
conf: &ClientConfig{
532+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
533+
Certificates: []string{td.Cert()},
534+
DiscoverDN: true,
535+
},
536+
username: "mallory",
537+
538+
wantErr: true,
539+
wantErrContains: "LDAP search for binddn 0 or not unique",
540+
},
541+
"fail: invalid search filter": {
542+
conf: &ClientConfig{
543+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
544+
Certificates: []string{td.Cert()},
545+
DiscoverDN: true,
546+
UserFilter: "({{.BadFilter}}={{.Username}})",
547+
},
548+
username: "alice",
549+
550+
wantErr: true,
551+
wantErrContains: "search failed due to template parsing error",
552+
},
553+
"success: discoverdn": {
554+
conf: &ClientConfig{
555+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
556+
Certificates: []string{td.Cert()},
557+
DiscoverDN: true,
558+
},
559+
username: "alice",
560+
561+
want: "cn=alice,ou=people,dc=example,dc=org",
562+
},
563+
"success: binddn and bindpass": {
564+
conf: &ClientConfig{
565+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
566+
Certificates: []string{td.Cert()},
567+
BindDN: "cn=bob,ou=people,dc=example,dc=org",
568+
BindPassword: "password",
569+
},
570+
username: "alice",
571+
572+
want: "cn=alice,ou=people,dc=example,dc=org",
573+
},
574+
"success: upndomain": {
575+
conf: &ClientConfig{
576+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
577+
Certificates: []string{td.Cert()},
578+
UPNDomain: "example.com",
579+
},
580+
username: "eve",
581+
582+
want: "eve@example.com",
583+
},
584+
"success: userdn": {
585+
conf: &ClientConfig{
586+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
587+
Certificates: []string{td.Cert()},
588+
UserDN: testdirectory.DefaultUserDN,
589+
},
590+
username: "alice",
591+
592+
want: "cn=alice,ou=people,dc=example,dc=org",
593+
},
594+
}
595+
596+
for name, tc := range cases {
597+
t.Run(name, func(t *testing.T) {
598+
assert, require := assert.New(t), require.New(t)
599+
c, err := NewClient(testCtx, tc.conf)
600+
require.NoError(err)
601+
err = c.connect(testCtx)
602+
require.NoError(err)
603+
defer func() { c.Close(testCtx) }()
604+
got, err := c.getUserBindDN(tc.username)
605+
if tc.wantErr {
606+
require.Error(err)
607+
if tc.wantErrIs != nil {
608+
assert.ErrorIs(err, tc.wantErrIs)
609+
}
610+
if tc.wantErrContains != "" {
611+
assert.Contains(err.Error(), tc.wantErrContains)
612+
}
613+
return
614+
}
615+
require.NoError(err)
616+
assert.Equal(tc.want, got)
617+
})
618+
}
619+
}
620+
621+
func TestClient_getUserDN(t *testing.T) {
622+
t.Parallel()
623+
testCtx := context.Background()
624+
logger := hclog.New(&hclog.LoggerOptions{
625+
Name: "test-logger",
626+
Level: hclog.Error,
627+
})
628+
td := testdirectory.Start(nil,
629+
testdirectory.WithDefaults(nil, &testdirectory.Defaults{AllowAnonymousBind: true}),
630+
testdirectory.WithLogger(t, logger),
631+
)
632+
users := testdirectory.NewUsers(t, []string{"alice", "bob"})
633+
users = append(
634+
users,
635+
testdirectory.NewUsers(
636+
t,
637+
[]string{"eve"},
638+
testdirectory.WithDefaults(t, &testdirectory.Defaults{UPNDomain: "example.com"}))...,
639+
)
640+
// Set up a duplicated user to test the case where the search returns multiple users
641+
users = append(
642+
users,
643+
testdirectory.NewUsers(
644+
t,
645+
[]string{"mallory", "mallory"},
646+
testdirectory.WithDefaults(t, &testdirectory.Defaults{UPNDomain: "example.com"}),
647+
)...,
648+
)
649+
td.SetUsers(users...)
650+
651+
tests := map[string]struct {
652+
conf *ClientConfig
653+
bindDN string
654+
username string
655+
656+
want string
657+
wantErr bool
658+
wantErrIs error
659+
wantErrContains string
660+
}{
661+
"fail: missing bind dn": {
662+
conf: &ClientConfig{
663+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
664+
Certificates: []string{td.Cert()},
665+
},
666+
bindDN: "",
667+
username: "alice",
668+
669+
wantErr: true,
670+
wantErrIs: ErrInvalidParameter,
671+
wantErrContains: "missing bind dn",
672+
},
673+
"fail: missing username": {
674+
conf: &ClientConfig{
675+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
676+
Certificates: []string{td.Cert()},
677+
},
678+
bindDN: fmt.Sprintf("%s=%s,%s", testdirectory.DefaultUserAttr, "bob", testdirectory.DefaultUserDN),
679+
username: "",
680+
681+
wantErr: true,
682+
wantErrIs: ErrInvalidParameter,
683+
wantErrContains: "missing username",
684+
},
685+
"fail: upn domain search fails to find user": {
686+
conf: &ClientConfig{
687+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
688+
Certificates: []string{td.Cert()},
689+
UPNDomain: "example.com",
690+
},
691+
bindDN: "userPrincipalName=nonexistent@example.com,ou=people,dc=example,dc=org",
692+
username: "nonexistent",
693+
694+
wantErr: true,
695+
wantErrContains: "LDAP search failed for detecting user",
696+
},
697+
"fail: upn domain search returns multiple users": {
698+
conf: &ClientConfig{
699+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
700+
Certificates: []string{td.Cert()},
701+
UPNDomain: "example.com",
702+
},
703+
bindDN: "userPrincipalName=mallory@example.com,ou=people,dc=example,dc=org",
704+
username: "mallory",
705+
706+
wantErr: true,
707+
wantErrContains: "LDAP search for user 0 or not unique",
708+
},
709+
"success: no upn domain": {
710+
conf: &ClientConfig{
711+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
712+
Certificates: []string{td.Cert()},
713+
},
714+
bindDN: "cn=alice,ou=people,dc=example,dc=org",
715+
username: "alice",
716+
717+
want: "cn=alice,ou=people,dc=example,dc=org",
718+
},
719+
"success: upn domain with samaccountname": {
720+
conf: &ClientConfig{
721+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
722+
Certificates: []string{td.Cert()},
723+
UPNDomain: "example.com",
724+
EnableSamaccountnameLogin: true,
725+
},
726+
bindDN: "userPrincipalName=eve@example.com,ou=people,dc=example,dc=org",
727+
username: "eve",
728+
729+
want: "userPrincipalName=eve@example.com,ou=people,dc=example,dc=org",
730+
},
731+
"success: upn domain without samaccountname": {
732+
conf: &ClientConfig{
733+
URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())},
734+
Certificates: []string{td.Cert()},
735+
UPNDomain: "example.com",
736+
},
737+
bindDN: "userPrincipalName=eve@example.com,ou=people,dc=example,dc=org",
738+
username: "eve",
739+
740+
want: "userPrincipalName=eve@example.com,ou=people,dc=example,dc=org",
741+
},
742+
}
743+
744+
for name, tc := range tests {
745+
t.Run(name, func(t *testing.T) {
746+
assert, require := assert.New(t), require.New(t)
747+
c, err := NewClient(testCtx, tc.conf)
748+
require.NoError(err)
749+
err = c.connect(testCtx)
750+
require.NoError(err)
751+
defer func() { c.Close(testCtx) }()
752+
got, err := c.getUserDN(tc.bindDN, tc.username)
753+
if tc.wantErr {
754+
require.Error(err)
755+
if tc.wantErrIs != nil {
756+
assert.ErrorIs(err, tc.wantErrIs)
757+
}
758+
if tc.wantErrContains != "" {
759+
assert.Contains(err.Error(), tc.wantErrContains)
760+
}
761+
return
762+
}
763+
require.NoError(err)
764+
assert.Equal(tc.want, got)
765+
})
766+
}
767+
}
768+
459769
func Test_sidBytesToString(t *testing.T) {
460770
testcases := map[string][]byte{
461771
"S-1-5-21-2127521184-1604012920-1887927527-72713": {0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x15, 0x00, 0x00, 0x00, 0xA0, 0x65, 0xCF, 0x7E, 0x78, 0x4B, 0x9B, 0x5F, 0xE7, 0x7C, 0x87, 0x70, 0x09, 0x1C, 0x01, 0x00},

0 commit comments

Comments
 (0)