From 3446dceb8dae07187fc9baf033fed9eecfe3b3ed Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Tue, 15 Dec 2015 10:18:04 -0500 Subject: [PATCH] fixed group detection bug for LDAP prune --- .../syncgroups/groupdetector/groupdetector.go | 14 +++++++-- .../groupdetector/groupdetector_test.go | 29 ++++++++++++++++--- .../valid_all_ldap_sync_delete_prune.txt | 27 +++++++++++++++++ test/extended/ldap_groups.sh | 12 ++++++-- 4 files changed, 73 insertions(+), 9 deletions(-) create mode 100644 test/extended/authentication/ldap/augmented-ad/valid_all_ldap_sync_delete_prune.txt diff --git a/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector.go b/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector.go index 10f41f104238..850e86ec9c3c 100644 --- a/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector.go +++ b/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector.go @@ -18,7 +18,7 @@ type GroupBasedDetector struct { } func (l *GroupBasedDetector) Exists(ldapGroupUID string) (bool, error) { - _, err := l.groupGetter.GroupEntryFor(ldapGroupUID) + group, err := l.groupGetter.GroupEntryFor(ldapGroupUID) if ldaputil.IsQueryOutOfBoundsError(err) || ldaputil.IsEntryNotFoundError(err) || ldaputil.IsNoSuchObjectError(err) { return false, nil } @@ -26,6 +26,10 @@ func (l *GroupBasedDetector) Exists(ldapGroupUID string) (bool, error) { return false, err } + if group == nil { + return false, nil + } + return true, nil } @@ -76,13 +80,17 @@ type CompoundDetector struct { } func (l *CompoundDetector) Exists(ldapGrouUID string) (bool, error) { - conclusion := false + if len(l.locators) == 0 { + return false, nil + } + + conclusion := true for _, locator := range l.locators { opinion, err := locator.Exists(ldapGrouUID) if err != nil { return false, err } - conclusion = conclusion || opinion + conclusion = conclusion && opinion } return conclusion, nil } diff --git a/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector_test.go b/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector_test.go index 95eea02f5ab6..930008d34318 100644 --- a/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector_test.go +++ b/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector_test.go @@ -43,9 +43,15 @@ func TestGroupBasedDetectorExists(t *testing.T) { expectedExists: false, }, { - name: "no error", + name: "no error, no entries", groupGetter: &puppetGetterExtractor{}, expectedErr: nil, + expectedExists: false, + }, + { + name: "no error, has entries", + groupGetter: &puppetGetterExtractor{returnVal: []*ldap.Entry{dummyEntry}}, + expectedErr: nil, expectedExists: true, }, } @@ -126,10 +132,16 @@ func TestCompoundDetectorExists(t *testing.T) { expectedErr error expectedExists bool }{ + { + name: "no detectors", + locators: []interfaces.LDAPGroupDetector{}, + expectedErr: nil, + expectedExists: false, + }, { name: "none fail to locate", locators: []interfaces.LDAPGroupDetector{ - NewGroupBasedDetector(&puppetGetterExtractor{}), + NewGroupBasedDetector(&puppetGetterExtractor{returnVal: []*ldap.Entry{dummyEntry}}), NewMemberBasedDetector(&puppetGetterExtractor{returnVal: []*ldap.Entry{dummyEntry}}), }, expectedErr: nil, @@ -142,10 +154,10 @@ func TestCompoundDetectorExists(t *testing.T) { NewMemberBasedDetector(&puppetGetterExtractor{returnVal: []*ldap.Entry{dummyEntry}}), }, expectedErr: nil, - expectedExists: true, + expectedExists: false, }, { - name: "all fail to locate", + name: "all fail to locate because of errors", locators: []interfaces.LDAPGroupDetector{ NewGroupBasedDetector(&errEntryNotFoundGetterExtractor{}), NewMemberBasedDetector(&errOutOfBoundsGetterExtractor{}), @@ -153,6 +165,15 @@ func TestCompoundDetectorExists(t *testing.T) { expectedErr: nil, expectedExists: false, }, + { + name: "all locate no entries", + locators: []interfaces.LDAPGroupDetector{ + NewGroupBasedDetector(&puppetGetterExtractor{}), + NewMemberBasedDetector(&puppetGetterExtractor{}), + }, + expectedErr: nil, + expectedExists: false, + }, { name: "one errors", locators: []interfaces.LDAPGroupDetector{ diff --git a/test/extended/authentication/ldap/augmented-ad/valid_all_ldap_sync_delete_prune.txt b/test/extended/authentication/ldap/augmented-ad/valid_all_ldap_sync_delete_prune.txt new file mode 100644 index 000000000000..cc127131194e --- /dev/null +++ b/test/extended/authentication/ldap/augmented-ad/valid_all_ldap_sync_delete_prune.txt @@ -0,0 +1,27 @@ +apiVersion: v1 +kind: Group +metadata: + annotations: + openshift.io/ldap.uid: cn=group2,ou=groups,ou=adextended,dc=example,dc=com + openshift.io/ldap.url: LDAP_SERVICE_IP:389 + creationTimestamp: null + labels: + openshift.io/ldap.host: LDAP_SERVICE_IP + name: extended-group2 +users: +- person1smith@example.com +- person2smith@example.com +- person3smith@example.com +apiVersion: v1 +kind: Group +metadata: + annotations: + openshift.io/ldap.uid: cn=group3,ou=groups,ou=adextended,dc=example,dc=com + openshift.io/ldap.url: LDAP_SERVICE_IP:389 + creationTimestamp: null + labels: + openshift.io/ldap.host: LDAP_SERVICE_IP + name: extended-group3 +users: +- person1smith@example.com +- person5smith@example.com diff --git a/test/extended/ldap_groups.sh b/test/extended/ldap_groups.sh index 9ee29bc4b942..513aed6d9046 100755 --- a/test/extended/ldap_groups.sh +++ b/test/extended/ldap_groups.sh @@ -216,12 +216,11 @@ for (( i=0; i<${#schema[@]}; i++ )); do openshift ex sync-groups --type=openshift --sync-config=sync-config.yaml --confirm compare_and_cleanup valid_all_ldap_sync_user_defined.txt - - echo -e "\tTEST: Sync all LDAP groups from LDAP server using DN as attribute whenever possible" openshift ex sync-groups --sync-config=sync-config-dn-everywhere.yaml --confirm compare_and_cleanup valid_all_ldap_sync_dn_everywhere.txt + # PRUNING echo -e "\tTEST: Sync all LDAP groups from LDAP server, change LDAP UID, then prune OpenShift groups" openshift ex sync-groups --sync-config=sync-config.yaml --confirm @@ -231,3 +230,12 @@ for (( i=0; i<${#schema[@]}; i++ )); do popd > /dev/null done + +# special test for ad-extended +pushd ${BASETMPDIR}/augmented-ad > /dev/null +echo -e "\tTEST: Sync all LDAP groups from LDAP server, remove LDAP group metadata entry, then prune OpenShift groups" +openshift ex sync-groups --sync-config=sync-config.yaml --confirm +ldapdelete -x -h $LDAP_SERVICE_IP -p 389 -D cn=Manager,dc=example,dc=com -w admin "${group1_ldapuid}" +openshift ex prune-groups --sync-config=sync-config.yaml --confirm +compare_and_cleanup valid_all_ldap_sync_delete_prune.txt +popd > /dev/null \ No newline at end of file