TO API v5 routes should not use priv levels anymore (#7819)

* TO API v5 routes should not use priv levels anymore

* add changelog

* fix tests, cleanup

* remove commented out code

* code review

* rename perm constants

CHANGELOG.md

@@ -90,6 +90,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - [#7814](https://github.com/apache/trafficcontrol/issues/7814) All Go components: Updated the module path to [`github.com/apache/trafficcontrol/v8`](https://pkg.go.dev/github.com/apache/trafficcontrol/v8). Module https://pkg.go.dev/github.com/apache/trafficcontrol will not receive further updates.
 
 ### Fixed
+- [#7819](https://github.com/apache/trafficcontrol/pull/7819) *Traffic Ops*: API v5 routes should not use `privLevel` comparisons.
 - [#7802](https://github.com/apache/trafficcontrol/pull/7802) *Traffic Control Health Client*: Fixed ReadMe.md typos and duplicates.
 - [#7764](https://github.com/apache/trafficcontrol/pull/7764) *Traffic Ops*: Collapsed DB migrations.
 - [#7767](https://github.com/apache/trafficcontrol/pull/7767) *Traffic Ops*: Fixed ASN update logic for APIv5.

lib/go-tc/constants.go

@@ -98,3 +98,15 @@ func (a AlertLevel) String() string {
 // CachegroupCoordinateNamePrefix is a string that all cache group coordinate
 // names are prefixed with.
 const CachegroupCoordinateNamePrefix = "from_cachegroup_"
+
+// PermParameterSecureRead is a string representing the permission to be able to read secure parameters.
+const PermParameterSecureRead = "PARAMETER:SECURE-READ"
+
+// PermSecureServerRead is a string representing the permission to be able to read secure server properties.
+const PermSecureServerRead = "SECURE-SERVER:READ"
+
+// PermCDNLocksDeleteOthers is a string representing the permission to be able to delete other users' CDN locks.
+const PermCDNLocksDeleteOthers = "CDN-LOCK:DELETE-OTHERS"
+
+// PermICDNUCDNOverride is a string representing the permission to be able to override the ucdn parameter.
+const PermICDNUCDNOverride = "ICDN:UCDN-OVERRIDE"

traffic_ops/traffic_ops_golang/cdn_lock/cdn_lock.go

@@ -232,16 +232,21 @@ func Delete(w http.ResponseWriter, r *http.Request) {
 	tx := inf.Tx.Tx
 	var result tc.CDNLock
 	var err error
-	adminPerms := inf.Config.RoleBasedPermissions && inf.User.Can("CDN-LOCK:DELETE-OTHERS")
+	var adminPerms bool
 
-	if adminPerms || inf.User.PrivLevel == auth.PrivLevelAdmin {
+	if (inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 4}) && inf.Config.RoleBasedPermissions) || inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 5}) {
+		adminPerms = inf.User.Can(tc.PermCDNLocksDeleteOthers)
+	} else {
+		adminPerms = inf.User.PrivLevel == auth.PrivLevelAdmin
+	}
+	if adminPerms {
 		err = inf.Tx.Tx.QueryRow(deleteAdminQuery, cdn).Scan(&result.UserName, &result.CDN, &result.Message, &result.Soft, pq.Array(&result.SharedUserNames), &result.LastUpdated)
 	} else {
 		err = inf.Tx.Tx.QueryRow(deleteQuery, cdn, inf.User.UserName).Scan(&result.UserName, &result.CDN, &result.Message, &result.Soft, pq.Array(&result.SharedUserNames), &result.LastUpdated)
 	}
 	if err != nil {
 		if errors.Is(err, sql.ErrNoRows) {
-			if inf.User.PrivLevel != auth.PrivLevelAdmin {
+			if !adminPerms {
 				api.HandleErr(w, r, tx, http.StatusForbidden, fmt.Errorf("deleting cdn lock with cdn name %s: operation forbidden", cdn), nil)
 				return
 			}

traffic_ops/traffic_ops_golang/cdni/shared.go

@@ -563,7 +563,7 @@ func checkBearerToken(bearerToken string, inf *api.APIInfo) (string, error) {
 	}
 
 	if ucdn == "" {
-		if inf.User.Can("ICDN:UCDN-OVERRIDE") {
+		if inf.User.Can(tc.PermICDNUCDNOverride) {
 			ucdn = inf.Params["ucdn"]
 			if ucdn == "" {
 				return "", errors.New("admin level ucdn requests require a ucdn query parameter")

traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go

@@ -716,7 +716,7 @@ func GetReadAssigned(w http.ResponseWriter, r *http.Request) {
 	defer inf.Close()
 
 	alerts := tc.Alerts{}
-	servers, err := read(inf.Tx, inf.IntParams["id"], inf.User)
+	servers, err := read(inf)
 	if err != nil {
 		alerts.AddNewAlert(tc.ErrorLevel, err.Error())
 		api.WriteAlerts(w, r, http.StatusInternalServerError, alerts)
@@ -767,7 +767,7 @@ func GetReadAssigned(w http.ResponseWriter, r *http.Request) {
 	api.WriteAlertsObj(w, r, http.StatusOK, alerts, servers)
 }
 
-func read(tx *sqlx.Tx, dsID int, user *auth.CurrentUser) ([]tc.DSServerV4, error) {
+func read(inf *api.APIInfo) ([]tc.DSServerV4, error) {
 	queryDataString :=
 		`,
 cg.name as cachegroup,
@@ -813,7 +813,8 @@ JOIN status st ON s.status = st.id
 JOIN type t ON s.type = t.id
 WHERE s.id in (select server from deliveryservice_server where deliveryservice = $1)`
 
-	idRows, err := tx.Queryx(fmt.Sprintf(queryFormatString, ""), dsID)
+	dsID := inf.IntParams["id"]
+	idRows, err := inf.Tx.Queryx(fmt.Sprintf(queryFormatString, ""), dsID)
 	if err != nil {
 		return nil, errors.New("error querying dss ids: " + err.Error())
 	}
@@ -828,12 +829,12 @@ WHERE s.id in (select server from deliveryservice_server where deliveryservice =
 		serverIDs = append(serverIDs, serverID)
 	}
 
-	serversMap, err := dbhelpers.GetServersInterfaces(serverIDs, tx.Tx)
+	serversMap, err := dbhelpers.GetServersInterfaces(serverIDs, inf.Tx.Tx)
 	if err != nil {
 		return nil, errors.New("unable to get server interfaces: " + err.Error())
 	}
 
-	rows, err := tx.Queryx(fmt.Sprintf(queryFormatString, queryDataString), dsID)
+	rows, err := inf.Tx.Queryx(fmt.Sprintf(queryFormatString, queryDataString), dsID)
 	if err != nil {
 		return nil, errors.New("error querying dss rows: " + err.Error())
 	}
@@ -883,7 +884,14 @@ WHERE s.id in (select server from deliveryservice_server where deliveryservice =
 			}
 		}
 
-		if user.PrivLevel < auth.PrivLevelAdmin {
+		canViewILOPswd := false
+		if (inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 4}) && inf.Config.RoleBasedPermissions) || inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 5}) {
+			canViewILOPswd = inf.User.Can(tc.PermSecureServerRead)
+		} else {
+			canViewILOPswd = inf.User.PrivLevel == auth.PrivLevelAdmin
+		}
+
+		if !canViewILOPswd {
 			s.ILOPassword = util.StrPtr("")
 		}
 		servers = append(servers, s)

traffic_ops/traffic_ops_golang/deliveryservice/servers/servers_test.go

@@ -31,6 +31,7 @@ import (
 	"github.com/apache/trafficcontrol/v8/lib/go-util"
 	"github.com/apache/trafficcontrol/v8/traffic_ops/traffic_ops_golang/api"
 	"github.com/apache/trafficcontrol/v8/traffic_ops/traffic_ops_golang/auth"
+	"github.com/apache/trafficcontrol/v8/traffic_ops/traffic_ops_golang/config"
 
 	"github.com/jmoiron/sqlx"
 	"github.com/lib/pq"
@@ -495,7 +496,19 @@ func TestReadServers(t *testing.T) {
 	mock.ExpectQuery("SELECT").WillReturnRows(rows)
 	mock.ExpectCommit()
 
-	actualSrvs, err := read(db.MustBegin(), dsID, &auth.CurrentUser{PrivLevel: 30})
+	params := make(map[string]int, 0)
+	params["id"] = dsID
+	inf := api.APIInfo{
+		Version: &api.Version{
+			Major: 5,
+			Minor: 0,
+		},
+		Tx:        db.MustBegin(),
+		IntParams: params,
+		User:      &auth.CurrentUser{PrivLevel: 30},
+		Config:    &config.Config{RoleBasedPermissions: true},
+	}
+	actualSrvs, err := read(&inf)
 	if err != nil {
 		t.Fatalf("an error '%s' occurred during read", err)
 	}

traffic_ops/traffic_ops_golang/profile/copy_test.go

@@ -37,6 +37,7 @@ import (
 	"github.com/apache/trafficcontrol/v8/traffic_ops/traffic_ops_golang/trafficvault/backends/disabled"
 
 	"github.com/jmoiron/sqlx"
+	"github.com/lib/pq"
 	sqlmock "gopkg.in/DATA-DOG/go-sqlmock.v1"
 )
 
@@ -102,10 +103,16 @@ func TestCopyProfileInvalidExistingProfile(t *testing.T) {
 
 			inf := api.APIInfo{
 				Tx: db.MustBegin(),
+				Version: &api.Version{
+					Major: 5,
+					Minor: 0,
+				},
 				Params: map[string]string{
 					"existing_profile": c.profile.Response.ExistingName,
 					"new_profile":      c.profile.Response.Name,
 				},
+				Config: &config.Config{RoleBasedPermissions: true},
+				User:   &auth.CurrentUser{Capabilities: pq.StringArray{tc.PermParameterSecureRead}},
 			}
 
 			errs := copyProfile(&inf, &c.profile.Response)

traffic_ops/traffic_ops_golang/profile/profiles.go

@@ -189,10 +189,19 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
 	}
 	rows.Close()
 	profileInterfaces := []interface{}{}
+	canReadSecureValue := false
+	inf := prof.APIInfo()
+	if inf != nil && inf.Version != nil {
+		if inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 4}) && inf.Config.RoleBasedPermissions {
+			canReadSecureValue = inf.User.Can(tc.PermParameterSecureRead)
+		} else {
+			canReadSecureValue = inf.User.PrivLevel == auth.PrivLevelAdmin
+		}
+	}
 	for _, profile := range profiles {
 		// Attach Parameters if the 'id' parameter is sent
 		if _, ok := prof.APIInfo().Params[IDQueryParam]; ok {
-			profile.Parameters, err = ReadParameters(prof.ReqInfo.Tx, prof.ReqInfo.User, *profile.ID)
+			profile.Parameters, err = ReadParameters(prof.ReqInfo.Tx, prof.ReqInfo.User, *profile.ID, canReadSecureValue)
 			if err != nil {
 				return nil, nil, errors.New("profile read reading parameters: " + err.Error()), http.StatusInternalServerError, nil
 			}
@@ -229,8 +238,7 @@ LEFT JOIN cdn c ON prof.cdn = c.id`
 	return query
 }
 
-func ReadParameters(tx *sqlx.Tx, user *auth.CurrentUser, profileID int) ([]tc.ParameterNullable, error) {
-	privLevel := user.PrivLevel
+func ReadParameters(tx *sqlx.Tx, user *auth.CurrentUser, profileID int, canReadSecureValue bool) ([]tc.ParameterNullable, error) {
 	queryValues := make(map[string]interface{})
 	queryValues["profile_id"] = profileID
 
@@ -252,7 +260,7 @@ func ReadParameters(tx *sqlx.Tx, user *auth.CurrentUser, profileID int) ([]tc.Pa
 		if param.Secure != nil {
 			isSecure = *param.Secure
 		}
-		if isSecure && (privLevel < auth.PrivLevelAdmin) {
+		if isSecure && !canReadSecureValue {
 			param.Value = &parameter.HiddenField
 		}
 		params = append(params, param)
@@ -429,10 +437,11 @@ func Read(w http.ResponseWriter, r *http.Request) {
 	}
 	rows.Close()
 	profileInterfaces := []interface{}{}
+
 	for _, p := range profileList {
 		// Attach Parameters if the 'param' parameter is sent
 		if _, ok := inf.Params["param"]; ok {
-			p.Parameters, err = ReadParameters(inf.Tx, inf.User, p.ID)
+			p.Parameters, err = ReadParameters(inf.Tx, inf.User, p.ID, inf.User.Can("PARAMETER-SECURE:READ"))
 			if err != nil {
 				api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("profile read: error reading parameters for a profile: %w", err))
 				return

traffic_ops/traffic_ops_golang/server/servers.go

@@ -928,7 +928,7 @@ JOIN server_profile sp ON s.id = sp.server`
 			return nil, serverCount, nil, fmt.Errorf("getting servers: %w", err), http.StatusInternalServerError, nil
 		}
 		if (version.GreaterThanOrEqualTo(&api.Version{Major: 4}) && roleBasedPerms) || version.GreaterThanOrEqualTo(&api.Version{Major: 5}) {
-			if !user.Can("SECURE-SERVER:READ") {
+			if !user.Can(tc.PermSecureServerRead) {
 				s.ILOPassword = &HiddenField
 				s.XMPPPasswd = &HiddenField
 			}

traffic_ops/traffic_ops_golang/systeminfo/system_info.go

@@ -38,10 +38,17 @@ func Get(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	defer inf.Close()
-	api.RespWriter(w, r, inf.Tx.Tx)(getSystemInfo(inf.Tx, inf.User.PrivLevel, time.Duration(inf.Config.DBQueryTimeoutSeconds)*time.Second))
+
+	canReadSecureValue := false
+	if (inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 4}) && inf.Config.RoleBasedPermissions) || inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 5}) {
+		canReadSecureValue = inf.User.Can(tc.PermParameterSecureRead)
+	} else {
+		canReadSecureValue = inf.User.PrivLevel == auth.PrivLevelAdmin
+	}
+	api.RespWriter(w, r, inf.Tx.Tx)(getSystemInfo(inf.Tx, inf.User.PrivLevel, time.Duration(inf.Config.DBQueryTimeoutSeconds)*time.Second, canReadSecureValue))
 }
 
-func getSystemInfo(tx *sqlx.Tx, privLevel int, timeout time.Duration) (*tc.SystemInfo, error) {
+func getSystemInfo(tx *sqlx.Tx, privLevel int, timeout time.Duration, canReadHiddenValue bool) (*tc.SystemInfo, error) {
 	q := `
 SELECT
   p.name,
@@ -64,7 +71,7 @@ WHERE
 		if err = rows.StructScan(&p); err != nil {
 			return nil, errors.New("sqlx scanning system info global parameters: " + err.Error())
 		}
-		if p.Secure != nil && *p.Secure && privLevel < auth.PrivLevelAdmin {
+		if p.Secure != nil && *p.Secure && !canReadHiddenValue {
 			continue
 		}
 		if p.Name != nil && p.Value != nil {

traffic_ops/traffic_ops_golang/systeminfo/system_info_test.go

@@ -102,7 +102,7 @@ func TestGetSystemInfo(t *testing.T) {
 		t.Fatalf("creating transaction: %v", err)
 	}
 
-	sysinfo, err := getSystemInfo(tx, auth.PrivLevelReadOnly, 20*time.Second)
+	sysinfo, err := getSystemInfo(tx, auth.PrivLevelReadOnly, 20*time.Second, false)
 	if err != nil {
 		t.Fatalf("getSystemInfo expected: nil error, actual: %v", err)
 	}