From 0dee60173db734691f2a6c8cf298ee90bbf8c544 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Tue, 12 Oct 2021 16:48:33 -0600 Subject: [PATCH 1/2] check for mismatch values for regionName in POST and PUT /phys_locations --- CHANGELOG.md | 1 + .../testing/api/v4/phys_locations_test.go | 50 +++++++++++++++++++ .../dbhelpers/db_helpers.go | 12 +++++ .../traffic_ops_golang/login/register.go | 1 - .../physlocation/phys_locations.go | 34 +++++++++++-- traffic_ops/v4-client/phys_location.go | 2 +- 6 files changed, 95 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9624e011a9..1efb874b1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - [#5893](https://github.com/apache/trafficcontrol/issues/5893) - A self signed certificate is created when an HTTPS delivery service is created or an HTTP delivery service is updated to HTTPS. - [#6255](https://github.com/apache/trafficcontrol/issues/6255) - Unreadable Prod Mode CDN Notifications in Traffic Portal - [#6259](https://github.com/apache/trafficcontrol/issues/6259) - Traffic Portal No Longer Allows Spaces in Server Object "Router Port Name" +- [#6175](https://github.com/apache/trafficcontrol/issues/6175) - POST request to /api/4.0/phys_locations accepts mismatch values for regionName. ### Changed - Updated `t3c` to request less unnecessary deliveryservice-server assignment and invalidation jobs data via new query params supported by Traffic Ops diff --git a/traffic_ops/testing/api/v4/phys_locations_test.go b/traffic_ops/testing/api/v4/phys_locations_test.go index c787689434..727aca8038 100644 --- a/traffic_ops/testing/api/v4/phys_locations_test.go +++ b/traffic_ops/testing/api/v4/phys_locations_test.go @@ -26,6 +26,7 @@ import ( "time" "github.com/apache/trafficcontrol/lib/go-rfc" + "github.com/apache/trafficcontrol/lib/go-tc" client "github.com/apache/trafficcontrol/traffic_ops/v4-client" ) @@ -50,6 +51,7 @@ func TestPhysLocations(t *testing.T) { header.Set(rfc.IfMatch, etag) UpdateTestPhysLocationsWithHeaders(t, header) GetTestPaginationSupportPhysLocation(t) + CreatePhysLocationWithMismatchedRegionNameAndID(t) }) } @@ -276,6 +278,54 @@ func DeleteTestPhysLocations(t *testing.T) { } } +func CreatePhysLocationWithMismatchedRegionNameAndID(t *testing.T) { + resp, _, err := TOSession.GetRegions(client.NewRequestOptions()) + if err != nil { + t.Errorf("Unexpected error getting regions: %v - alerts: %+v", err, resp.Alerts) + } + if len(resp.Response) < 2 { + t.Fatalf("expected at least two regions to be returned, but got none") + } + physLocation := tc.PhysLocation{ + Address: "100 blah lane", + City: "foo", + Comments: "comment", + Email: "bar@foobar.com", + Name: "testPhysicalLocation", + Phone: "111-222-3333", + RegionID: resp.Response[0].ID, + RegionName: resp.Response[1].Name, + ShortName: "testLocation1", + State: "CO", + Zip: "80602", + } + _, _, err = TOSession.CreatePhysLocation(physLocation, client.NewRequestOptions()) + if err == nil { + t.Fatalf("expected an error about mismatched region name and ID, but got nothing") + } + + physLocation.RegionName = resp.Response[0].Name + _, _, err = TOSession.CreatePhysLocation(physLocation, client.NewRequestOptions()) + if err != nil { + t.Fatalf("expected no error while creating phys location, but got %v", err) + } + + opts := client.NewRequestOptions() + opts.QueryParameters.Set("name", "testPhysicalLocation") + response, _, err := TOSession.GetPhysLocations(opts) + if err != nil { + t.Fatalf("cannot get Physical Location by name 'testPhysicalLocation': %v - alerts: %+v", err, resp.Alerts) + } + if len(response.Response) != 1 { + t.Fatalf("Expected exactly one Physical Location to exist with name 'testPhysicalLocation', found: %d", len(resp.Response)) + } + + _, _, err = TOSession.DeletePhysLocation(response.Response[0].ID, client.NewRequestOptions()) + if err != nil { + t.Errorf("error deleteing physical location 'testPhysicalLocation': %v", err) + } +} + func GetTestPaginationSupportPhysLocation(t *testing.T) { opts := client.NewRequestOptions() opts.QueryParameters.Set("orderby", "id") diff --git a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go index f891a9c30e..0961f40244 100644 --- a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go +++ b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go @@ -1716,3 +1716,15 @@ func GetCDNNameDomain(cdnID int, tx *sql.Tx) (string, string, error) { } return cdnName, cdnDomain, nil } + +// GetRegionNameFromID returns the name of the region associated with the supplied ID. +func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { + var regionName string + if err := tx.QueryRow(`SELECT name FROM region WHERE id = $1`, regionID).Scan(®ionName); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return regionName, false, nil + } + return regionName, false, fmt.Errorf("querying region name from ID: %w", err) + } + return regionName, true, nil +} diff --git a/traffic_ops/traffic_ops_golang/login/register.go b/traffic_ops/traffic_ops_golang/login/register.go index a10d7e4217..8f664a3bed 100644 --- a/traffic_ops/traffic_ops_golang/login/register.go +++ b/traffic_ops/traffic_ops_golang/login/register.go @@ -225,7 +225,6 @@ func RegisterUser(w http.ResponseWriter, r *http.Request) { } else { req.Email = reqV4.Email req.TenantID = reqV4.TenantID - dbhelpers.GetRoleIDFromName(tx, reqV4.Role) roleID, ok, err := dbhelpers.GetRoleIDFromName(tx, reqV4.Role) if err != nil { api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("error fetching ID from role name: %w", err)) diff --git a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go index da8922dd27..d605cfb0d1 100644 --- a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go +++ b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go @@ -20,6 +20,8 @@ package physlocation */ import ( + "errors" + "fmt" "net/http" "strconv" "time" @@ -116,9 +118,35 @@ JOIN region r ON pl.region = r.id ` + where + orderBy + pagination + select max(last_updated) as t from last_deleted l where l.table_name='phys_location') as res` } -func (pl *TOPhysLocation) Update(h http.Header) (error, error, int) { return api.GenericUpdate(h, pl) } -func (pl *TOPhysLocation) Create() (error, error, int) { return api.GenericCreate(pl) } -func (pl *TOPhysLocation) Delete() (error, error, int) { return api.GenericDelete(pl) } +// MatchRegionNameAndID checks to see if the supplied region name and ID in the phys_location body correspond to each other. +func (pl *TOPhysLocation) MatchRegionNameAndID() (error, error, int) { + if pl.RegionName != nil { + regionName, ok, err := dbhelpers.GetRegionNameFromID(pl.APIInfo().Tx.Tx, *pl.RegionID) + if err != nil { + return nil, fmt.Errorf("error fetching name from region ID: %w", err), http.StatusInternalServerError + } else if !ok { + return errors.New("no such region"), nil, http.StatusNotFound + } + if regionName != *pl.RegionName { + return errors.New("region name and ID do not match"), nil, http.StatusBadRequest + } + } + return nil, nil, http.StatusOK +} + +func (pl *TOPhysLocation) Update(h http.Header) (error, error, int) { + if userErr, sysErr, statusCode := pl.MatchRegionNameAndID(); userErr != nil || sysErr != nil { + return userErr, sysErr, statusCode + } + return api.GenericUpdate(h, pl) +} +func (pl *TOPhysLocation) Create() (error, error, int) { + if userErr, sysErr, statusCode := pl.MatchRegionNameAndID(); userErr != nil || sysErr != nil { + return userErr, sysErr, statusCode + } + return api.GenericCreate(pl) +} +func (pl *TOPhysLocation) Delete() (error, error, int) { return api.GenericDelete(pl) } func selectQuery() string { return ` diff --git a/traffic_ops/v4-client/phys_location.go b/traffic_ops/v4-client/phys_location.go index 5f43dea666..c9c94f3f76 100644 --- a/traffic_ops/v4-client/phys_location.go +++ b/traffic_ops/v4-client/phys_location.go @@ -30,7 +30,7 @@ func (to *Session) CreatePhysLocation(pl tc.PhysLocation, opts RequestOptions) ( if pl.RegionID == 0 && pl.RegionName != "" { regionOpts := NewRequestOptions() regionOpts.QueryParameters.Set("name", pl.RegionName) - regions, reqInf, err := to.GetRegions(opts) + regions, reqInf, err := to.GetRegions(regionOpts) if err != nil { err = fmt.Errorf("resolving Region name '%s' to an ID", pl.RegionName) return regions.Alerts, reqInf, err From 27fff50aef0cb7e932a68eeb7145b1e02fba1d97 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Mon, 18 Oct 2021 11:48:31 -0600 Subject: [PATCH 2/2] modify update prerequisites --- .../traffic_ops_golang/physlocation/phys_locations.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go index d605cfb0d1..d6fbd85907 100644 --- a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go +++ b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go @@ -134,12 +134,7 @@ func (pl *TOPhysLocation) MatchRegionNameAndID() (error, error, int) { return nil, nil, http.StatusOK } -func (pl *TOPhysLocation) Update(h http.Header) (error, error, int) { - if userErr, sysErr, statusCode := pl.MatchRegionNameAndID(); userErr != nil || sysErr != nil { - return userErr, sysErr, statusCode - } - return api.GenericUpdate(h, pl) -} +func (pl *TOPhysLocation) Update(h http.Header) (error, error, int) { return api.GenericUpdate(h, pl) } func (pl *TOPhysLocation) Create() (error, error, int) { if userErr, sysErr, statusCode := pl.MatchRegionNameAndID(); userErr != nil || sysErr != nil { return userErr, sysErr, statusCode