[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5397: Return the correct error while trying to update the root tenant

[GitBox <gi...@apache.org>]

zrhoffman commented on a change in pull request #5397:
URL: https://github.com/apache/trafficcontrol/pull/5397#discussion_r552126130



##########
File path: traffic_ops/traffic_ops_golang/apitenant/tenant.go
##########
@@ -184,13 +186,16 @@ func (ten *TOTenant) IsTenantAuthorized(user *auth.CurrentUser) (bool, error) {
 		// get current parentID to check if it's being changed
 		var parentID int
 		tx := ten.APIInfo().Tx.Tx
-		err = tx.QueryRow(`SELECT parent_id FROM tenant WHERE id = ` + strconv.Itoa(*ten.ID)).Scan(&parentID)
-		if err != nil {
-			return false, err
-		}
-		if parentID == *ten.ParentID {
-			// parent not being changed
-			return ok, err
+		// If its the rot tenant, dont check for parent

Review comment:
       * its
   * rot
   * dont

##########
File path: traffic_ops/traffic_ops_golang/apitenant/tenant.go
##########
@@ -202,7 +207,12 @@ 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) { return api.GenericUpdate(h, tn) }
+func (tn *TOTenant) Update(h http.Header) (error, error, int) {
+	if tn.Name != nil && *tn.Name == rootName {
+		return errors.New("cannot update the root tenant"), nil, http.StatusBadRequest

Review comment:
       400-level errors should include a description of why the server rejected the request.

##########
File path: traffic_ops/testing/api/v3/tenants_test.go
##########
@@ -170,6 +171,26 @@ func UpdateTestTenants(t *testing.T) {
 
 }
 
+func UpdateTestRootTenant(t *testing.T) {
+	expected := "cannot update the root tenant"
+	// Retrieve the Tenant by name so we can get the id for the Update
+	name := "root"
+	modTenant, _, err := TOSession.TenantByNameWithHdr(name, nil)
+	if err != nil {
+		t.Errorf("cannot GET Tenant by name: %s - %v", name, err)
+	}
+
+	modTenant.Active = false
+	modTenant.ParentID = modTenant.ID
+	_, err = TOSession.UpdateTenantWithHdr(strconv.Itoa(modTenant.ID), modTenant, nil)
+	if err == nil {
+		t.Fatalf("expected an error when trying to update the 'root' tenant, but got nothing")
+	}
+	if !strings.Contains(err.Error(), expected) {
+		t.Errorf("expected error detail to contain %s, but got %s", expected, err.Error())
+	}

Review comment:
       Our API tests checked the text of error messages before we added the `ReqInf.StatusCode` field. Now, asserting that the status code is 400-level is sufficient (and way more maintainable).




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org