fixed group detection bug for LDAP prune

openshift · Dec 15, 2015 · cc05f7f · cc05f7f
1 parent 1904cc3
commit cc05f7f
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 7 deletions.
diff --git a/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector.go b/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector.go
@@ -18,14 +18,18 @@ 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
 	}
 	if err != nil {
 		return false, err
 	}
 
+	if group == nil {
+		return false, nil
+	}
+
 	return true, nil
 }
 
@@ -76,13 +80,13 @@ type CompoundDetector struct {
 }
 
 func (l *CompoundDetector) Exists(ldapGrouUID string) (bool, error) {
-	conclusion := false
+	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
@@ -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,
 		},
 	}
@@ -129,7 +135,7 @@ func TestCompoundDetectorExists(t *testing.T) {
 		{
 			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,17 +148,26 @@ 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{}),
 			},
 			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
@@ -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
@@ -229,5 +229,14 @@ for (( i=0; i<${#schema[@]}; i++ )); do
 	openshift ex prune-groups --sync-config=sync-config.yaml --confirm
 	compare_and_cleanup valid_all_ldap_sync_prune.txt
 
+	# special-case test for ad-extended
+	if [[ "${current_schema}" == augmented-ad ]]; then
+		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		
+	fi
+
     popd > /dev/null
 done