diff --git a/CHANGELOG.md b/CHANGELOG.md index 907507f66..c25f156c3 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/govcd/api_vcd_test.go b/govcd/api_vcd_test.go index b2fe0b030..11fa03381 100644 --- a/govcd/api_vcd_test.go +++ b/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 { diff --git a/govcd/catalog_test.go b/govcd/catalog_test.go index 41ea54118..bcb132294 100644 --- a/govcd/catalog_test.go +++ b/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) +} diff --git a/govcd/common_test.go b/govcd/common_test.go index 374869df6..09dd3ac43 100644 --- a/govcd/common_test.go +++ b/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", diff --git a/govcd/org.go b/govcd/org.go index e471bf56f..9e89dd02e 100644 --- a/govcd/org.go +++ b/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) diff --git a/govcd/org_test.go b/govcd/org_test.go index 1e54d7760..b2c9c535e 100644 --- a/govcd/org_test.go +++ b/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 -}