V2.12.0 regression fix for org.GetCatalogByName and `org.GetCatalog…

…ById` (#389)

* go-vcloud-director does not find catalogs shared from other Orgs in methods org.GetCatalogByName and org.GetCatalogById

This addon fixes that, adds some tests including a test with uses Org Admin user and adds a changelog note for 2.12.1 bugfix release.

CHANGELOG.md

@@ -1,3 +1,10 @@
+## 2.12.1 (5 July, 2021)
+
+BUGS FIXED:
+* org.GetCatalogByName and org.GetCatalogById could not retrieve shared catalogs from different Orgs 
+  [#389](https://github.com/vmware/go-vcloud-director/pull/389)
+
+
 ## 2.12.0 (June 30, 2021)
 
 * Added method `vdc.QueryEdgeGateway` [#364](https://github.com/vmware/go-vcloud-director/pull/364)

govcd/api_vcd_test.go

@@ -1852,7 +1852,6 @@ func skipOpenApiEndpointTest(vcd *TestVCD, check *C, endpoint string) {
 }
 
 // newOrgUserConnection creates a new Org User and returns a connection to it
-//lint:ignore U1000 For future usage - Allows writing tests that require multiple users
 func newOrgUserConnection(adminOrg *AdminOrg, userName, password, href string, insecure bool) (*VCDClient, error) {
 	u, err := url.ParseRequestURI(href)
 	if err != nil {

govcd/catalog_test.go

@@ -597,3 +597,189 @@ func (vcd *TestVCD) TestGetVappTemplateByHref(check *C) {
 	check.Assert(vappTemplate.VAppTemplate.Type, Equals, types.MimeVAppTemplate)
 	check.Assert(vappTemplate.VAppTemplate.Name, Equals, catalogItem.CatalogItem.Name)
 }
+
+// Test_GetCatalogByNameSharedCatalog creates a separate Org and VDC just to create Catalog and share it with main Org
+// One should be able to find shared catalogs from different Organizations
+func (vcd *TestVCD) Test_GetCatalogByNameSharedCatalog(check *C) {
+	fmt.Printf("Running: %s\n", check.TestName())
+
+	newOrg1, vdc, sharedCatalog := createSharedCatalogInNewOrg(vcd, check, check.TestName())
+
+	// Try to find the catalog inside Org which owns it - newOrg1
+	catalogByName, err := newOrg1.GetCatalogByName(sharedCatalog.Catalog.Name, true)
+	check.Assert(err, IsNil)
+	check.Assert(catalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+
+	// Try to find the catalog in another Org with which this catalog is shared (vcd.Org)
+	sharedCatalogByName, err := vcd.org.GetCatalogByName(sharedCatalog.Catalog.Name, false)
+	check.Assert(err, IsNil)
+	check.Assert(sharedCatalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+
+	cleanupCatalogOrgVdc(check, sharedCatalog, vdc, vcd, newOrg1)
+}
+
+// Test_GetCatalogByIdSharedCatalog creates a separate Org and VDC just to create Catalog and share it with main Org
+// One should be able to find shared catalogs from different Organizations
+func (vcd *TestVCD) Test_GetCatalogByIdSharedCatalog(check *C) {
+	fmt.Printf("Running: %s\n", check.TestName())
+
+	newOrg1, vdc, sharedCatalog := createSharedCatalogInNewOrg(vcd, check, check.TestName())
+
+	// Try to find the sharedCatalog inside Org which owns it - newOrg1
+	catalogById, err := newOrg1.GetCatalogById(sharedCatalog.Catalog.ID, true)
+	check.Assert(err, IsNil)
+	check.Assert(catalogById.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+
+	// Try to find the sharedCatalog in another Org with which this sharedCatalog is shared (vcd.Org)
+	sharedCatalogById, err := vcd.org.GetCatalogById(sharedCatalog.Catalog.ID, false)
+	check.Assert(err, IsNil)
+	check.Assert(sharedCatalogById.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+
+	cleanupCatalogOrgVdc(check, sharedCatalog, vdc, vcd, newOrg1)
+}
+
+// Test_GetCatalogByNamePrefersLocal tests that local catalog (in the same Org) is prioritised against shared catalogs
+// in other Orgs. It does so by creating another Org with shared Catalog named just like the one in testing catalog
+func (vcd *TestVCD) Test_GetCatalogByNamePrefersLocal(check *C) {
+	fmt.Printf("Running: %s\n", check.TestName())
+
+	// Create a catalog  in new org with exactly the same name as in vcd.Org
+	newOrg1, vdc, sharedCatalog := createSharedCatalogInNewOrg(vcd, check, vcd.config.VCD.Catalog.Name)
+
+	// Make sure that the Owner Org HREF is the local one for vcd.Org catalog named vcd.config.VCD.Catalog.Name
+	catalogByNameInTestOrg, err := vcd.org.GetCatalogByName(vcd.config.VCD.Catalog.Name, true)
+	check.Assert(err, IsNil)
+	check.Assert(catalogByNameInTestOrg.parent.orgName(), Equals, vcd.org.Org.Name)
+
+	// Make sure that the Owner Org HREF is the local one for vcd.Org catalog named vcd.config.VCD.Catalog.Name
+	catalogByNameInNewOrg, err := newOrg1.GetCatalogByName(vcd.config.VCD.Catalog.Name, true)
+	check.Assert(err, IsNil)
+	check.Assert(catalogByNameInNewOrg.parent.orgName(), Equals, newOrg1.Org.Name)
+
+	cleanupCatalogOrgVdc(check, sharedCatalog, vdc, vcd, newOrg1)
+}
+
+// Test_GetCatalogByNameSharedCatalogOrgUser additionally tests GetOrgByName and GetOrgById using a custom created Org
+// Admin user. It tests the following cases:
+// * System user must be able to retrieve any catalog - shared or unshared from another Org
+// * Org Admin user must be able to retrieve catalog in his own Org
+// * Org Admin user must be able to retrieve shared catalog from another Org
+// * Org admin user must not be able to retrieve unshared catalog from another Org
+func (vcd *TestVCD) Test_GetCatalogByXSharedCatalogOrgUser(check *C) {
+	fmt.Printf("Running: %s\n", check.TestName())
+	newOrg1, vdc, sharedCatalog := createSharedCatalogInNewOrg(vcd, check, check.TestName())
+
+	// Create one more additional catalog which is not shared
+	unsharedCatalog, err := newOrg1.CreateCatalog("unshared-catalog", check.TestName())
+	check.Assert(err, IsNil)
+	AddToCleanupList(unsharedCatalog.Catalog.Name, "catalog", newOrg1.Org.Name, check.TestName())
+
+	// Try to find the catalog inside Org which owns it - newOrg1
+	catalogByName, err := newOrg1.GetCatalogByName(sharedCatalog.Catalog.Name, true)
+	check.Assert(err, IsNil)
+	check.Assert(catalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+
+	// Try to find the catalog in another Org with which this catalog is shared (vcd.Org)
+	sharedCatalogByName, err := vcd.org.GetCatalogByName(sharedCatalog.Catalog.Name, false)
+	check.Assert(err, IsNil)
+	check.Assert(sharedCatalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+
+	// Try to find unshared catalog from another Org with System user
+	systemUnsharedCatalogByName, err := vcd.org.GetCatalogByName(unsharedCatalog.Catalog.Name, true)
+	check.Assert(err, IsNil)
+	check.Assert(systemUnsharedCatalogByName.Catalog.ID, Equals, unsharedCatalog.Catalog.ID)
+
+	// Create an Org Admin user and test that it can find catalog as well
+	adminOrg, err := vcd.client.GetAdminOrgByName(vcd.config.VCD.Org)
+	check.Assert(err, IsNil)
+	orgAdminClient, err := newOrgUserConnection(adminOrg, "test-user", "CHANGE-ME", vcd.config.Provider.Url, true)
+	check.Assert(err, IsNil)
+	orgAsOrgUser, err := orgAdminClient.GetOrgByName(vcd.config.VCD.Org)
+	check.Assert(err, IsNil)
+
+	// Find a catalog in the same Org using Org Admin user
+	orgAdminCatalogByNameSameOrg, err := orgAsOrgUser.GetCatalogByName(vcd.config.VCD.Catalog.Name, false)
+	check.Assert(err, IsNil)
+	check.Assert(orgAdminCatalogByNameSameOrg.Catalog.Name, Equals, vcd.config.VCD.Catalog.Name)
+
+	orgAdminCatalogByIdSameOrg, err := orgAsOrgUser.GetCatalogById(orgAdminCatalogByNameSameOrg.Catalog.ID, false)
+	check.Assert(err, IsNil)
+	check.Assert(orgAdminCatalogByIdSameOrg.Catalog.Name, Equals, orgAdminCatalogByNameSameOrg.Catalog.Name)
+	check.Assert(orgAdminCatalogByIdSameOrg.Catalog.ID, Equals, orgAdminCatalogByNameSameOrg.Catalog.ID)
+
+	// Find a shared catalog from another Org using Org Admin user
+	orgAdminCatalogByName, err := orgAsOrgUser.GetCatalogByName(sharedCatalog.Catalog.Name, false)
+	check.Assert(err, IsNil)
+	check.Assert(orgAdminCatalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+	check.Assert(orgAdminCatalogByName.Catalog.ID, Equals, sharedCatalog.Catalog.ID)
+
+	orgAdminCatalogById, err := orgAsOrgUser.GetCatalogById(sharedCatalog.Catalog.ID, false)
+	check.Assert(err, IsNil)
+	check.Assert(orgAdminCatalogById.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+	check.Assert(orgAdminCatalogById.Catalog.ID, Equals, sharedCatalog.Catalog.ID)
+
+	// Try to find unshared catalog from another Org with Org admin user and expect an ErrorEntityNotFound
+	_, err = orgAsOrgUser.GetCatalogByName(unsharedCatalog.Catalog.Name, true)
+	check.Assert(ContainsNotFound(err), Equals, true)
+
+	_, err = orgAsOrgUser.GetCatalogById(unsharedCatalog.Catalog.ID, true)
+	check.Assert(ContainsNotFound(err), Equals, true)
+
+	// Cleanup
+	err = unsharedCatalog.Delete(true, true)
+	check.Assert(err, IsNil)
+
+	cleanupCatalogOrgVdc(check, sharedCatalog, vdc, vcd, newOrg1)
+}
+
+func createSharedCatalogInNewOrg(vcd *TestVCD, check *C, newCatalogName string) (*Org, *Vdc, Catalog) {
+	newOrgName1 := spawnTestOrg(vcd, check, "org")
+
+	newOrg1, err := vcd.client.GetOrgByName(newOrgName1)
+	check.Assert(err, IsNil)
+
+	// Spawn a VDC inside newly created Org so that there is storage to create new catalog
+	vdc := spawnTestVdc(vcd, check, newOrgName1)
+
+	catalog, err := newOrg1.CreateCatalog(newCatalogName, "Catalog for testing")
+	check.Assert(err, IsNil)
+	AddToCleanupList(newCatalogName, "catalog", newOrgName1, check.TestName())
+
+	// Share new Catalog in newOrgName1 with default test Org vcd.Org
+	readOnly := "ReadOnly"
+	accessControl := &types.ControlAccessParams{
+		IsSharedToEveryone:  false,
+		EveryoneAccessLevel: &readOnly,
+		AccessSettings: &types.AccessSettingList{
+			AccessSetting: []*types.AccessSetting{&types.AccessSetting{
+				Subject: &types.LocalSubject{
+					HREF: vcd.org.Org.HREF,
+					Name: vcd.org.Org.Name,
+					Type: types.MimeOrg,
+				},
+				AccessLevel: "ReadOnly",
+			}},
+		},
+	}
+	err = catalog.SetAccessControl(accessControl, false)
+	check.Assert(err, IsNil)
+
+	return newOrg1, vdc, catalog
+}
+
+func cleanupCatalogOrgVdc(check *C, sharedCatalog Catalog, vdc *Vdc, vcd *TestVCD, newOrg1 *Org) {
+	// Cleanup catalog, vdc and org
+	err := sharedCatalog.Delete(true, true)
+	check.Assert(err, IsNil)
+
+	// There are cases where it just takes a a few seconds after catalog deletion when one can delete VDC
+	time.Sleep(2 * time.Second)
+
+	err = vdc.DeleteWait(true, true)
+	check.Assert(err, IsNil)
+
+	adminOrg, err := vcd.client.GetAdminOrgByName(newOrg1.Org.Name)
+	check.Assert(err, IsNil)
+	err = adminOrg.Delete(true, true)
+	check.Assert(err, IsNil)
+}

govcd/common_test.go

@@ -793,6 +793,20 @@ func spawnTestVdc(vcd *TestVCD, check *C, adminOrgName string) *Vdc {
 	return vdc
 }
 
+// spawnTestOrg spawns an Org to be used in tests
+func spawnTestOrg(vcd *TestVCD, check *C, nameSuffix string) string {
+	newOrg, err := vcd.client.GetAdminOrgByName(vcd.config.VCD.Org)
+	check.Assert(err, IsNil)
+	newOrgName := check.TestName() + "-" + nameSuffix
+	task, err := CreateOrg(vcd.client, newOrgName, newOrgName, newOrgName, newOrg.AdminOrg.OrgSettings, true)
+	check.Assert(err, IsNil)
+	err = task.WaitTaskCompletion()
+	check.Assert(err, IsNil)
+	AddToCleanupList(newOrgName, "org", "", check.TestName())
+
+	return newOrgName
+}
+
 func getVdcProviderVdcHref(vcd *TestVCD, check *C) string {
 	results, err := vcd.client.QueryWithNotEncodedParams(nil, map[string]string{
 		"type":   "providerVdc",

govcd/org.go

@@ -415,9 +415,11 @@ func (org *Org) queryOrgVdcById(vdcId string) (*types.QueryResultOrgVdcRecordTyp
 // queryCatalogByName returns a single CatalogRecord
 func (org *Org) queryCatalogByName(catalogName string) (*types.CatalogRecord, error) {
 	filterMap := map[string]string{
-		"org":     org.Org.HREF, // Org ID is not allowed for non System
-		"orgName": org.Org.Name,
-		"name":    catalogName,
+		// Not injecting `org` or `orgName` here because shared catalogs may also appear here and they would have different
+		// parent Org
+		// "org":     org.Org.HREF,
+		// "orgName": org.Org.Name,
+		"name": catalogName,
 	}
 	allCatalogs, err := queryCatalogList(org.client, filterMap)
 	if err != nil {
@@ -428,19 +430,44 @@ func (org *Org) queryCatalogByName(catalogName string) (*types.CatalogRecord, er
 		return nil, ErrorEntityNotFound
 	}
 
+	// To conform with this API standard it would be best to return an error if more than 1 item is found, but because
+	// previous method of getting Catalog by Name returned the first result we are doing the same here
+	// if len(allCatalogs) > 1 {
+	// 	return nil, fmt.Errorf("found more than 1 Catalog with Name '%s'", catalogName)
+	// }
+
+	var localCatalog *types.CatalogRecord
+	// if multiple results are found - return the one defined in `org` (local)
 	if len(allCatalogs) > 1 {
-		return nil, fmt.Errorf("found more than 1 VDC with Name '%s'", catalogName)
+		util.Logger.Printf("[DEBUG] org.queryCatalogByName found %d Catalogs by name '%s'", len(allCatalogs), catalogName)
+		for _, catalog := range allCatalogs {
+			util.Logger.Printf("[DEBUG] org.queryCatalogByName found a Catalog by name '%s' in Org '%s'", catalogName, catalog.OrgName)
+			if catalog.OrgName == org.Org.Name {
+				util.Logger.Printf("[DEBUG] org.queryCatalogByName Catalog '%s' is local for Org '%s'. Prioritising it",
+					catalogName, org.Org.Name)
+				// Not interrupting the loop here to still dump all results to logs
+				localCatalog = catalog
+			}
+		}
 	}
 
+	// local catalog was found - return it
+	if localCatalog != nil {
+		return localCatalog, nil
+	}
+
+	// If only one catalog is found or multiple catalogs with no local ones - return the first one
 	return allCatalogs[0], nil
 }
 
 // queryCatalogById returns a single QueryResultOrgVdcRecordType
 func (org *Org) queryCatalogById(catalogId string) (*types.CatalogRecord, error) {
 	filterMap := map[string]string{
-		"org":     org.Org.HREF,
-		"orgName": org.Org.Name,
-		"id":      catalogId,
+		// Not injecting `org` or `orgName` here because shared catalogs may also appear here and they would have different
+		// parent Org
+		// "org":     org.Org.HREF,
+		// "orgName": org.Org.Name,
+		"id": catalogId,
 	}
 	allCatalogs, err := queryCatalogList(org.client, filterMap)
 

govcd/org_test.go

@@ -1107,17 +1107,3 @@ func validateQueryOrgVdcResults(vcd *TestVCD, check *C, name, orgName string, ex
 		fmt.Println()
 	}
 }
-
-// spawnTestOrg spawns an Org to be used in tests
-func spawnTestOrg(vcd *TestVCD, check *C, nameSuffix string) string {
-	newOrg, err := vcd.client.GetAdminOrgByName(vcd.config.VCD.Org)
-	check.Assert(err, IsNil)
-	newOrgName := check.TestName() + "-" + nameSuffix
-	task, err := CreateOrg(vcd.client, newOrgName, newOrgName, newOrgName, newOrg.AdminOrg.OrgSettings, true)
-	check.Assert(err, IsNil)
-	err = task.WaitTaskCompletion()
-	check.Assert(err, IsNil)
-	AddToCleanupList(newOrgName, "org", "", check.TestName())
-
-	return newOrgName
-}