[#142976749] fixed defect where ldap users were being added even if a…

…lready in role and map was properly being updated for remove-users function

organization/users.go

@@ -59,11 +59,12 @@ func (m *UserManager) UpdateOrgUsers(config *ldap.Config, uaacUsers map[string]s
 			return err
 		}
 		for _, user := range ldapUsers {
-			err = m.updateLdapUser(config, updateUsersInput.OrgGUID, updateUsersInput.Role, updateUsersInput.OrgName, uaacUsers, user)
-			if err != nil {
-				return err
-			}
 			if _, ok := orgUsers[user.UserID]; !ok {
+				err = m.updateLdapUser(config, updateUsersInput.OrgGUID, updateUsersInput.Role, updateUsersInput.OrgName, uaacUsers, user)
+				if err != nil {
+					return err
+				}
+			} else {
 				delete(orgUsers, user.UserID)
 			}
 		}

organization/users_test.go

@@ -73,7 +73,7 @@ var _ = Describe("given UserManager", func() {
 			_, ok := uaacUsers["user-id"]
 			Ω(ok).Should(BeTrue())
 		})
-		It("update ldap group users where users are not uaac", func() {
+		It("update ldap group users where users are in uaac", func() {
 			config := &l.Config{
 				Enabled: true,
 				Origin:  "ldap",
@@ -107,6 +107,38 @@ var _ = Describe("given UserManager", func() {
 			Ω(ok).Should(BeTrue())
 		})
 
+		It("update ldap group users where users are in uaac and in org", func() {
+			config := &l.Config{
+				Enabled: true,
+				Origin:  "ldap",
+			}
+			uaacUsers := make(map[string]string)
+			uaacUsers["user-id"] = "user-id"
+			orgUsers := make(map[string]string)
+			orgUsers["user-id"] = "user-id"
+			updateUsersInput := UpdateUsersInput{
+				OrgGUID:       "my-org-guid",
+				Role:          "my-role",
+				LdapGroupName: "ldap-group-name",
+			}
+
+			ldapGroupUsers := []l.User{l.User{
+				UserDN: "user-dn",
+				UserID: "user-id",
+				Email:  "[email protected]",
+			}}
+
+			mockCloudController.EXPECT().GetCFUsers("my-org-guid", "organizations", "my-role").Return(orgUsers, nil)
+			mockLdap.EXPECT().GetUserIDs(config, "ldap-group-name").Return(ldapGroupUsers, nil)
+
+			err := userManager.UpdateOrgUsers(config, uaacUsers, updateUsersInput)
+			Ω(err).Should(BeNil())
+
+			Ω(len(uaacUsers)).Should(BeEquivalentTo(1))
+			_, ok := uaacUsers["user-id"]
+			Ω(ok).Should(BeTrue())
+		})
+
 		It("update ldap users where users are not in uaac", func() {
 			config := &l.Config{
 				Enabled: true,
@@ -225,6 +257,35 @@ var _ = Describe("given UserManager", func() {
 			Ω(ok).Should(BeTrue())
 		})
 
+		It("update users where users are in uaac and in org", func() {
+			config := &l.Config{
+				Enabled: true,
+				Origin:  "ldap",
+			}
+			uaacUsers := make(map[string]string)
+			uaacUsers["user-1"] = "user-1"
+			uaacUsers["user-2"] = "user-2"
+			orgUsers := make(map[string]string)
+			orgUsers["user-1"] = "user-1"
+			orgUsers["user-2"] = "user-2"
+			updateUsersInput := UpdateUsersInput{
+				OrgGUID: "my-org-guid",
+				Role:    "my-role",
+				Users:   []string{"user-1", "user-2"},
+			}
+
+			mockCloudController.EXPECT().GetCFUsers("my-org-guid", "organizations", "my-role").Return(orgUsers, nil)
+
+			err := userManager.UpdateOrgUsers(config, uaacUsers, updateUsersInput)
+			Ω(err).Should(BeNil())
+
+			Ω(len(uaacUsers)).Should(BeEquivalentTo(2))
+			_, ok := uaacUsers["user-1"]
+			Ω(ok).Should(BeTrue())
+			_, ok = uaacUsers["user-2"]
+			Ω(ok).Should(BeTrue())
+		})
+
 		It("update users where users are not in uaac", func() {
 			config := &l.Config{
 				Enabled: true,

space/users.go

@@ -60,11 +60,12 @@ func (m *UserManager) UpdateSpaceUsers(config *ldap.Config, uaacUsers map[string
 			return err
 		}
 		for _, user := range ldapUsers {
-			err = m.updateLdapUser(config, updateUsersInput.SpaceGUID, updateUsersInput.OrgGUID, updateUsersInput.Role, updateUsersInput.OrgName, updateUsersInput.SpaceName, uaacUsers, user)
-			if err != nil {
-				return err
-			}
 			if _, ok := spaceUsers[user.UserID]; !ok {
+				err = m.updateLdapUser(config, updateUsersInput.SpaceGUID, updateUsersInput.OrgGUID, updateUsersInput.Role, updateUsersInput.OrgName, updateUsersInput.SpaceName, uaacUsers, user)
+				if err != nil {
+					return err
+				}
+			} else {
 				delete(spaceUsers, user.UserID)
 			}
 		}

space/users_test.go

@@ -74,7 +74,7 @@ var _ = Describe("given SpaceManager", func() {
 			_, ok := uaacUsers["user-id"]
 			Ω(ok).Should(BeTrue())
 		})
-		It("update ldap group users where users are not uaac", func() {
+		It("update ldap group users where users are in uaac", func() {
 			config := &l.Config{
 				Enabled: true,
 				Origin:  "ldap",
@@ -108,6 +108,38 @@ var _ = Describe("given SpaceManager", func() {
 			Ω(ok).Should(BeTrue())
 		})
 
+		It("update ldap group users where users are in uaac and already in space", func() {
+			config := &l.Config{
+				Enabled: true,
+				Origin:  "ldap",
+			}
+			uaacUsers := make(map[string]string)
+			uaacUsers["user-id"] = "user-id"
+			spaceUsers := make(map[string]string)
+			spaceUsers["user-id"] = "user-id"
+			updateUsersInput := UpdateUsersInput{
+				SpaceGUID:     "my-space-guid",
+				OrgGUID:       "my-org-guid",
+				Role:          "my-role",
+				LdapGroupName: "ldap-group-name",
+			}
+
+			ldapGroupUsers := []l.User{l.User{
+				UserDN: "user-dn",
+				UserID: "user-id",
+				Email:  "[email protected]",
+			}}
+
+			mockCloudController.EXPECT().GetCFUsers("my-space-guid", "spaces", "my-role").Return(spaceUsers, nil)
+			mockLdap.EXPECT().GetUserIDs(config, "ldap-group-name").Return(ldapGroupUsers, nil)
+
+			err := userManager.UpdateSpaceUsers(config, uaacUsers, updateUsersInput)
+			Ω(err).Should(BeNil())
+			Ω(len(uaacUsers)).Should(BeEquivalentTo(1))
+			_, ok := uaacUsers["user-id"]
+			Ω(ok).Should(BeTrue())
+		})
+
 		It("update ldap users where users are not in uaac", func() {
 			config := &l.Config{
 				Enabled: true,
@@ -228,6 +260,36 @@ var _ = Describe("given SpaceManager", func() {
 			Ω(ok).Should(BeTrue())
 		})
 
+		It("update users where users are in uaac and in a space", func() {
+			config := &l.Config{
+				Enabled: true,
+				Origin:  "ldap",
+			}
+			uaacUsers := make(map[string]string)
+			uaacUsers["user-1"] = "user-1"
+			uaacUsers["user-2"] = "user-2"
+			spaceUsers := make(map[string]string)
+			spaceUsers["user-1"] = "user-1"
+			spaceUsers["user-2"] = "user-2"
+			updateUsersInput := UpdateUsersInput{
+				SpaceGUID: "my-space-guid",
+				OrgGUID:   "my-org-guid",
+				Role:      "my-role",
+				Users:     []string{"user-1", "user-2"},
+			}
+
+			mockCloudController.EXPECT().GetCFUsers("my-space-guid", "spaces", "my-role").Return(spaceUsers, nil)
+
+			err := userManager.UpdateSpaceUsers(config, uaacUsers, updateUsersInput)
+			Ω(err).Should(BeNil())
+
+			Ω(len(uaacUsers)).Should(BeEquivalentTo(2))
+			_, ok := uaacUsers["user-1"]
+			Ω(ok).Should(BeTrue())
+			_, ok = uaacUsers["user-2"]
+			Ω(ok).Should(BeTrue())
+		})
+
 		It("update users where users are not in uaac", func() {
 			config := &l.Config{
 				Enabled: true,