You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficcontrol.apache.org by ma...@apache.org on 2021/01/29 16:10:51 UTC
[trafficcontrol] branch master updated: Bugfix/5405 parent child
tenant update (#5474)
This is an automated email from the ASF dual-hosted git repository.
mattjackson pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficcontrol.git
The following commit(s) were added to refs/heads/master by this push:
new 26b2b75 Bugfix/5405 parent child tenant update (#5474)
26b2b75 is described below
commit 26b2b758e720dd0a845a41266e141a08cb455e18
Author: Taylor Clayton Frey <ta...@gmail.com>
AuthorDate: Fri Jan 29 09:10:35 2021 -0700
Bugfix/5405 parent child tenant update (#5474)
* Prevent tenant update to chose child as new parent
As it stands currently, there is a bug where you can update a Tenant
and select a child of said tenant to be the new parent.
This causes a circular reference in the database with the adjanceny
list and then prevents the child and parent from being returned in the
recursive CTE, which makes it appear as though those values have been
deleted.
> Note that should this happen it is still possible to salvage the data
by manually editing the `tenants` table and fixing the incorrect
parent/child relationship.
To prevent the problem, a check is done to ensure the chosen ParentID
does not reside in the current list of children when attempting to
update the Tenant.
* Add bugfix notice to Changelog
* Use camel case for variable names
* Fix camelcase in test file.
Co-authored-by: Taylor Frey <ta...@comcast.com>
---
.gitignore | 1 +
CHANGELOG.md | 1 +
traffic_ops/traffic_ops_golang/apitenant/tenant.go | 74 ++++++++++---
.../traffic_ops_golang/apitenant/tenant_test.go | 120 +++++++++++++++++++++
4 files changed, 179 insertions(+), 17 deletions(-)
diff --git a/.gitignore b/.gitignore
index 6bbff0b..f6775d3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -53,6 +53,7 @@ local.tar.gz
*.sublime-project
*.sublime-workspace
.vscode/
+__debug_bin
*.code-workspace
*.pydevproject
.idea/
diff --git a/CHANGELOG.md b/CHANGELOG.md
index fa12261..d173696 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -42,6 +42,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- [#5364](https://github.com/apache/trafficcontrol/issues/5364) - Cascade server deletes to delete corresponding IP addresses and interfaces
- [#5390](https://github.com/apache/trafficcontrol/issues/5390) - Improve the way TO deals with delivery service server assignments
- [#5339](https://github.com/apache/trafficcontrol/issues/5339) - Ensure Changelog entries for SSL key changes
+- [#5405](https://github.com/apache/trafficcontrol/issues/5405) - Prevent Tenant update from choosing child as new parent
- [#5461](https://github.com/apache/trafficcontrol/issues/5461) - Fixed steering endpoint to be ordered consistently
### Changed
diff --git a/traffic_ops/traffic_ops_golang/apitenant/tenant.go b/traffic_ops/traffic_ops_golang/apitenant/tenant.go
index bd2b980..49c1d75 100644
--- a/traffic_ops/traffic_ops_golang/apitenant/tenant.go
+++ b/traffic_ops/traffic_ops_golang/apitenant/tenant.go
@@ -25,6 +25,10 @@ import (
"database/sql"
"errors"
"fmt"
+ "net/http"
+ "strconv"
+ "time"
+
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-tc/tovalidate"
"github.com/apache/trafficcontrol/lib/go-util"
@@ -34,9 +38,6 @@ import (
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/tenant"
validation "github.com/go-ozzo/ozzo-validation"
"github.com/lib/pq"
- "net/http"
- "strconv"
- "time"
)
const rootName = `root`
@@ -47,23 +48,23 @@ type TOTenant struct {
tc.TenantNullable
}
-func (v *TOTenant) GetLastUpdated() (*time.Time, bool, error) {
- return api.GetLastUpdated(v.APIInfo().Tx, *v.ID, "tenant")
+func (ten *TOTenant) GetLastUpdated() (*time.Time, bool, error) {
+ return api.GetLastUpdated(ten.APIInfo().Tx, *ten.ID, "tenant")
}
-func (v *TOTenant) SetLastUpdated(t tc.TimeNoMod) { v.LastUpdated = &t }
-func (v *TOTenant) InsertQuery() string { return insertQuery() }
-func (v *TOTenant) SelectMaxLastUpdatedQuery(where, orderBy, pagination, tableName string) string {
+func (ten *TOTenant) SetLastUpdated(t tc.TimeNoMod) { ten.LastUpdated = &t }
+func (ten *TOTenant) InsertQuery() string { return insertQuery() }
+func (ten *TOTenant) SelectMaxLastUpdatedQuery(where, orderBy, pagination, tableName string) string {
return `SELECT max(t) from (
SELECT max(last_updated) as t from ` + tableName + ` q ` + where + orderBy + pagination +
` UNION ALL
select max(last_updated) as t from last_deleted l where l.table_name='` + tableName + `') as res`
}
-func (v *TOTenant) NewReadObj() interface{} { return &tc.TenantNullable{} }
-func (v *TOTenant) SelectQuery() string {
- return selectQuery(v.APIInfo().User.TenantID)
+func (ten *TOTenant) NewReadObj() interface{} { return &tc.TenantNullable{} }
+func (ten *TOTenant) SelectQuery() string {
+ return selectQuery(ten.APIInfo().User.TenantID)
}
-func (v *TOTenant) ParamColumns() map[string]dbhelpers.WhereColumnInfo {
+func (ten *TOTenant) ParamColumns() map[string]dbhelpers.WhereColumnInfo {
return map[string]dbhelpers.WhereColumnInfo{
"active": dbhelpers.WhereColumnInfo{Column: "q.active", Checker: nil},
"id": dbhelpers.WhereColumnInfo{Column: "q.id", Checker: api.IsInt},
@@ -72,7 +73,7 @@ func (v *TOTenant) ParamColumns() map[string]dbhelpers.WhereColumnInfo {
"parent_name": dbhelpers.WhereColumnInfo{Column: "p.name", Checker: nil},
}
}
-func (v *TOTenant) UpdateQuery() string { return updateQuery() }
+func (ten *TOTenant) UpdateQuery() string { return updateQuery() }
// GetID wraps the ID member with null checking
// Part of the Identifier interface
@@ -131,7 +132,7 @@ func (ten TOTenant) Validate() error {
return util.JoinErrs(tovalidate.ToErrors(errs))
}
-func (tn *TOTenant) Create() (error, error, int) { return api.GenericCreate(tn) }
+func (ten *TOTenant) Create() (error, error, int) { return api.GenericCreate(ten) }
func (ten *TOTenant) Read(h http.Header, useIMS bool) ([]interface{}, error, error, int, *time.Time) {
if ten.APIInfo().User.TenantID == auth.TenantIDInvalid {
@@ -207,11 +208,50 @@ func (ten *TOTenant) IsTenantAuthorized(user *auth.CurrentUser) (bool, error) {
return tenant.IsResourceAuthorizedToUserTx(*ten.ParentID, user, ten.APIInfo().Tx.Tx)
}
-func (tn *TOTenant) Update(h http.Header) (error, error, int) {
- if tn.Name != nil && *tn.Name == rootName {
+// Update wraps tenant validation and the generic API Update call into a single call.
+func (ten *TOTenant) Update(h http.Header) (error, error, int) {
+
+ userErr, sysErr, statusCode := ten.isUpdatable()
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, statusCode
+ }
+
+ return api.GenericUpdate(h, ten)
+}
+
+// isUpdatable peforms validation on the fields for the Tenant, such as ensuring
+// the tenant cannot be modified if it is root, or that it cannot convert its own child
+// to its own parent. This is different than the basic validation rules performed in
+// Validate() as it pertains to specific business logic, not generic API rules.
+func (ten *TOTenant) isUpdatable() (error, error, int) {
+ if ten.Name != nil && *ten.Name == rootName {
return errors.New("trying to change the root tenant, which is immutable"), nil, http.StatusBadRequest
}
- return api.GenericUpdate(h, tn)
+
+ // Perform SelectQuery
+ vals := []tc.TenantNullable{}
+ query := selectQuery(*ten.ID)
+ rows, err := ten.APIInfo().Tx.Queryx(query)
+ if err != nil {
+ return nil, errors.New("querying " + ten.GetType() + ": " + err.Error()), http.StatusInternalServerError
+ }
+ defer rows.Close()
+
+ for rows.Next() {
+ var v tc.TenantNullable
+ if err = rows.StructScan(&v); err != nil {
+ return nil, errors.New("scanning " + ten.GetType() + ": " + err.Error()), http.StatusInternalServerError
+ }
+ vals = append(vals, v)
+ }
+
+ // Ensure the new desired ParentID does not exist in the susequent list of Children
+ for _, val := range vals {
+ if *ten.ParentID == *val.ID {
+ return errors.New("trying to set existing child as new parent"), nil, http.StatusBadRequest
+ }
+ }
+ return nil, nil, http.StatusOK
}
func (ten *TOTenant) Delete() (error, error, int) {
diff --git a/traffic_ops/traffic_ops_golang/apitenant/tenant_test.go b/traffic_ops/traffic_ops_golang/apitenant/tenant_test.go
index 405809c..d66fce5 100644
--- a/traffic_ops/traffic_ops_golang/apitenant/tenant_test.go
+++ b/traffic_ops/traffic_ops_golang/apitenant/tenant_test.go
@@ -20,9 +20,15 @@ package apitenant
*/
import (
+ "net/http"
+ "strconv"
"testing"
+ "github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
+ "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/test"
+ "github.com/jmoiron/sqlx"
+ "gopkg.in/DATA-DOG/go-sqlmock.v1"
)
func TestInterfaces(t *testing.T) {
@@ -48,3 +54,117 @@ func TestInterfaces(t *testing.T) {
t.Errorf("Tenant must be Tenantable")
}
}
+
+// TestIsUpdateable is a test to ensure attempts to update a tenant
+// pass validation beforehand.
+func TestIsUpdateable(t *testing.T) {
+
+ mockDB, mock, err := sqlmock.New()
+ if err != nil {
+ t.Fatalf("an error '%s' was not expected when opening a stub database connection", err)
+ }
+ defer mockDB.Close()
+
+ db := sqlx.NewDb(mockDB, "sqlmock")
+ defer db.Close()
+
+ // First test, attempt to change root
+ root := getRootTestTenant()
+ userErr, _, statusCode := root.isUpdatable()
+ if userErr == nil && statusCode != http.StatusBadRequest {
+ t.Errorf("Should not be able to update root tenant. userErr = %s, statuscode = %d", userErr, statusCode)
+ }
+
+ // Second test, attempt to change Child's (ID:7) parent from ID:3 to ID:1 (root)
+ child := getValidChildTenant()
+ updateParentIDValue := 1
+ child.ParentID = &updateParentIDValue // set parent ID as would be done through the GenericUpdate and Keys call
+
+ mock.ExpectBegin()
+ mock.ExpectQuery("SELECT")
+
+ child.ReqInfo = &api.APIInfo{Tx: db.MustBegin(), Params: map[string]string{"id": strconv.Itoa(*child.ID)}}
+ userErr, _, statusCode = child.isUpdatable()
+ if userErr != nil && statusCode != http.StatusOK {
+ t.Errorf("Should be able to update child to new parent (from Parent to Root). userErr = %s, statuscode = %d", userErr, statusCode)
+ }
+
+ // Third test, attempt to change Parent from ID:3 to ID:7 (Child)
+ parent := getValidParentTenant()
+ updateParentIDValue = 7
+ parent.ParentID = &updateParentIDValue // set parent ID as would be done through the GenericUpdate and Keys call
+ cols := test.ColsFromStructByTag("db", tc.TenantNullable{})
+ rows := sqlmock.NewRows(cols).AddRow(
+ child.ID,
+ child.Name,
+ child.Active,
+ child.LastUpdated,
+ child.ParentID,
+ child.ParentName,
+ )
+
+ mock.ExpectBegin()
+ mock.ExpectQuery("SELECT").WillReturnRows(rows)
+
+ parent.ReqInfo = &api.APIInfo{Tx: db.MustBegin(), Params: map[string]string{"id": strconv.Itoa(*parent.ID)}}
+ userErr, _, statusCode = parent.isUpdatable()
+ if userErr == nil && statusCode != http.StatusBadRequest {
+ t.Errorf("Should NOT be able to update parent to own child (from Parent to Child). userErr = %s, statuscode = %d", userErr, statusCode)
+ }
+
+}
+
+func getValidChildTenant() *TOTenant {
+ ten := &TOTenant{}
+ var tenid int = 7
+ var tenname string = "Child"
+ var tenact bool = true
+ var tenpid int = 3
+ var tenpaname string = "Parent"
+ ten.TenantNullable = tc.TenantNullable{
+ ID: &tenid,
+ Name: &tenname,
+ Active: &tenact,
+ LastUpdated: tc.NewTimeNoMod(),
+ ParentID: &tenpid,
+ ParentName: &tenpaname,
+ }
+ ten.ReqInfo = &api.APIInfo{}
+ return ten
+}
+
+func getValidParentTenant() *TOTenant {
+ ten := &TOTenant{}
+ var tenid int = 3
+ var tenname string = "Parent"
+ var tenact bool = true
+ var tenpid int = 1
+ var tenpaname string = "root"
+ ten.TenantNullable = tc.TenantNullable{
+ ID: &tenid,
+ Name: &tenname,
+ Active: &tenact,
+ LastUpdated: tc.NewTimeNoMod(),
+ ParentID: &tenpid,
+ ParentName: &tenpaname,
+ }
+ ten.ReqInfo = &api.APIInfo{}
+ return ten
+}
+
+func getRootTestTenant() *TOTenant {
+ ten := &TOTenant{}
+ var tenid int = 1
+ var tenname string = "root"
+ var tenact bool = true
+ ten.TenantNullable = tc.TenantNullable{
+ ID: &tenid,
+ Name: &tenname,
+ Active: &tenact,
+ LastUpdated: tc.NewTimeNoMod(),
+ ParentID: nil,
+ ParentName: nil,
+ }
+ ten.ReqInfo = &api.APIInfo{}
+ return ten
+}