You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficcontrol.apache.org by rs...@apache.org on 2023/10/03 16:09:38 UTC
[trafficcontrol] branch 8.0.x updated: TO API v5 routes should not use priv levels anymore (#7819)
This is an automated email from the ASF dual-hosted git repository.
rshah pushed a commit to branch 8.0.x
in repository https://gitbox.apache.org/repos/asf/trafficcontrol.git
The following commit(s) were added to refs/heads/8.0.x by this push:
new cedcd2c033 TO API v5 routes should not use priv levels anymore (#7819)
cedcd2c033 is described below
commit cedcd2c033ef8cde189b54c47ff59bbeed1be976
Author: Srijeet Chatterjee <30...@users.noreply.github.com>
AuthorDate: Thu Sep 28 12:39:05 2023 -0600
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
(cherry picked from commit 5081509d5f0b42f5c50621deb07521c7db6d863a)
---
CHANGELOG.md | 1 +
lib/go-tc/constants.go | 12 ++++++++++++
traffic_ops/traffic_ops_golang/cdn_lock/cdn_lock.go | 11 ++++++++---
traffic_ops/traffic_ops_golang/cdni/shared.go | 2 +-
.../deliveryservice/servers/servers.go | 20 ++++++++++++++------
.../deliveryservice/servers/servers_test.go | 15 ++++++++++++++-
traffic_ops/traffic_ops_golang/profile/copy_test.go | 7 +++++++
traffic_ops/traffic_ops_golang/profile/profiles.go | 19 ++++++++++++++-----
traffic_ops/traffic_ops_golang/server/servers.go | 2 +-
.../traffic_ops_golang/systeminfo/system_info.go | 13 ++++++++++---
.../systeminfo/system_info_test.go | 2 +-
11 files changed, 83 insertions(+), 21 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 30a7c43871..81739dfa2e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -85,6 +85,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.
diff --git a/lib/go-tc/constants.go b/lib/go-tc/constants.go
index 188d604bed..461f83181d 100644
--- a/lib/go-tc/constants.go
+++ b/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"
diff --git a/traffic_ops/traffic_ops_golang/cdn_lock/cdn_lock.go b/traffic_ops/traffic_ops_golang/cdn_lock/cdn_lock.go
index 84646cb904..b243caff0d 100644
--- a/traffic_ops/traffic_ops_golang/cdn_lock/cdn_lock.go
+++ b/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
}
diff --git a/traffic_ops/traffic_ops_golang/cdni/shared.go b/traffic_ops/traffic_ops_golang/cdni/shared.go
index c824297e2b..d6ee011cde 100644
--- a/traffic_ops/traffic_ops_golang/cdni/shared.go
+++ b/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")
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go b/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
index d5ee648b43..b0695cb7a6 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
+++ b/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)
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers_test.go b/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers_test.go
index 2dfa26fca8..e2d9b670e2 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers_test.go
+++ b/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)
}
diff --git a/traffic_ops/traffic_ops_golang/profile/copy_test.go b/traffic_ops/traffic_ops_golang/profile/copy_test.go
index 8b338af7dc..bed7f006c1 100644
--- a/traffic_ops/traffic_ops_golang/profile/copy_test.go
+++ b/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)
diff --git a/traffic_ops/traffic_ops_golang/profile/profiles.go b/traffic_ops/traffic_ops_golang/profile/profiles.go
index d2cb11b926..df706be8fb 100644
--- a/traffic_ops/traffic_ops_golang/profile/profiles.go
+++ b/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 = ¶meter.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
diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go
index 99ba0715e3..510a58692d 100644
--- a/traffic_ops/traffic_ops_golang/server/servers.go
+++ b/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
}
diff --git a/traffic_ops/traffic_ops_golang/systeminfo/system_info.go b/traffic_ops/traffic_ops_golang/systeminfo/system_info.go
index dedfb71d99..9e6c731067 100644
--- a/traffic_ops/traffic_ops_golang/systeminfo/system_info.go
+++ b/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 {
diff --git a/traffic_ops/traffic_ops_golang/systeminfo/system_info_test.go b/traffic_ops/traffic_ops_golang/systeminfo/system_info_test.go
index 337c4d14f8..658540284e 100644
--- a/traffic_ops/traffic_ops_golang/systeminfo/system_info_test.go
+++ b/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)
}