[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5930: Locking down snap/ queue/ update status endpoints for a CDN

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

ocket8888 commented on a change in pull request #5930:
URL: https://github.com/apache/trafficcontrol/pull/5930#discussion_r650217757



##########
File path: traffic_ops/testing/api/v4/cdn_locks_test.go
##########
@@ -199,3 +260,227 @@ func AdminCdnLocks(t *testing.T) {
 		t.Fatalf("expected a 200 status code, but got %d instead", reqInf.StatusCode)
 	}
 }
+
+func SnapshotWithLock(t *testing.T) {
+	resp, _, err := TOSession.GetTenants(client.RequestOptions{})
+	if err != nil {
+		t.Fatalf("could not GET tenants: %v", err)
+	}
+	if len(resp.Response) == 0 {
+		t.Fatalf("didn't get any tenant in response")
+	}
+
+	// Create a new user with operations level privileges
+	user1 := tc.User{
+		Username:             util.StrPtr("lock_user1"),
+		RegistrationSent:     tc.TimeNoModFromTime(time.Now()),
+		LocalPassword:        util.StrPtr("test_pa$$word"),
+		ConfirmLocalPassword: util.StrPtr("test_pa$$word"),
+		RoleName:             util.StrPtr("operations"),
+	}
+	user1.Email = util.StrPtr("lockuseremail@domain.com")
+	user1.TenantID = util.IntPtr(resp.Response[0].ID)
+	user1.FullName = util.StrPtr("firstName LastName")
+	_, _, err = TOSession.CreateUser(user1, client.RequestOptions{})
+	if err != nil {
+		t.Fatalf("could not create test user with username: %s", *user1.Username)
+	}
+	defer ForceDeleteTestUsersByUsernames(t, []string{"lock_user1"})
+
+	// Establish a session with the newly created non admin level user
+	userSession, _, err := client.LoginWithAgent(Config.TrafficOps.URL, *user1.Username, *user1.LocalPassword, true, "to-api-v4-client-tests", false, toReqTimeout)
+	if err != nil {
+		t.Fatalf("could not login with user lock_user1: %v", err)
+	}
+
+	cdn := getCDNName(t)
+
+	// Currently, no user has a lock on the "bar" CDN, so when "lock_user1", which does not have the lock on CDN "bar", tries to snap it, this should pass
+	opts := client.NewRequestOptions()
+	opts.QueryParameters.Set("cdn", cdn)
+	_, _, err = userSession.SnapshotCRConfig(opts)
+	if err != nil {
+		t.Errorf("expected no error while snapping cdn %s by user %s, but got %v", cdn, *user1.Username, err)
+	}
+
+	// Create a lock for this user
+	_, _, err = userSession.CreateCDNLock(tc.CDNLock{
+		CDN:     cdn,
+		Message: util.StrPtr("test lock"),
+		Soft:    util.BoolPtr(true),
+	}, client.RequestOptions{})
+	if err != nil {
+		t.Fatalf("couldn't create cdn lock: %v", err)
+	}
+
+	// "lock_user1", which has the lock on CDN "bar", tries to snap it -> this should pass
+	_, _, err = userSession.SnapshotCRConfig(opts)
+	if err != nil {
+		t.Errorf("expected no error while snapping cdn %s by user %s, but got %v", cdn, *user1.Username, err)
+	}
+
+	// Admin user, which doesn't have the lock on the CDN "bar", is trying to snap it -> this should fail
+	_, reqInf, err := TOSession.SnapshotCRConfig(opts)
+	if err == nil {
+		t.Errorf("expected error while snapping cdn %s by user admin, but got nothing", cdn)
+	}
+	if reqInf.StatusCode != http.StatusForbidden {
+		t.Fatalf("expected a 403 status code, but got %d instead", reqInf.StatusCode)

Review comment:
       Is this error fatal? I think the rest of the test can proceed if this condition isn't met

##########
File path: traffic_ops/testing/api/v4/cdn_locks_test.go
##########
@@ -45,6 +49,63 @@ func getCDNName(t *testing.T) string {
 	return cdnResp.Response[0].Name
 }
 
+func getCDNNameAndServerID(t *testing.T) (string, int) {
+	serverID := -1
+	cdnResp, _, err := TOSession.GetCDNs(client.RequestOptions{})
+	if err != nil {
+		t.Fatalf("couldn't get CDNs: %v", err)
+	}
+	if len(cdnResp.Response) < 1 {
+		t.Fatalf("no valid CDNs in response")
+	}
+	for _, cdn := range cdnResp.Response {
+		opts := client.NewRequestOptions()
+		opts.QueryParameters.Set("cdn", strconv.Itoa(cdn.ID))
+		serversResp, _, err := TOSession.GetServers(opts)
+		if err != nil {
+			t.Errorf("could not get servers for cdn %s: %v", cdn.Name, err)
+		}
+		if len(serversResp.Response) != 0 {
+			serverID = *serversResp.Response[0].ID

Review comment:
       This will segfault if Traffic Ops returns a malformed response that has a `null` or undefined server ID

##########
File path: traffic_ops/testing/api/v4/cdn_locks_test.go
##########
@@ -199,3 +260,227 @@ func AdminCdnLocks(t *testing.T) {
 		t.Fatalf("expected a 200 status code, but got %d instead", reqInf.StatusCode)
 	}
 }
+
+func SnapshotWithLock(t *testing.T) {
+	resp, _, err := TOSession.GetTenants(client.RequestOptions{})
+	if err != nil {
+		t.Fatalf("could not GET tenants: %v", err)
+	}
+	if len(resp.Response) == 0 {
+		t.Fatalf("didn't get any tenant in response")
+	}
+
+	// Create a new user with operations level privileges
+	user1 := tc.User{
+		Username:             util.StrPtr("lock_user1"),
+		RegistrationSent:     tc.TimeNoModFromTime(time.Now()),
+		LocalPassword:        util.StrPtr("test_pa$$word"),
+		ConfirmLocalPassword: util.StrPtr("test_pa$$word"),
+		RoleName:             util.StrPtr("operations"),
+	}
+	user1.Email = util.StrPtr("lockuseremail@domain.com")
+	user1.TenantID = util.IntPtr(resp.Response[0].ID)
+	user1.FullName = util.StrPtr("firstName LastName")
+	_, _, err = TOSession.CreateUser(user1, client.RequestOptions{})
+	if err != nil {
+		t.Fatalf("could not create test user with username: %s", *user1.Username)
+	}
+	defer ForceDeleteTestUsersByUsernames(t, []string{"lock_user1"})
+
+	// Establish a session with the newly created non admin level user
+	userSession, _, err := client.LoginWithAgent(Config.TrafficOps.URL, *user1.Username, *user1.LocalPassword, true, "to-api-v4-client-tests", false, toReqTimeout)
+	if err != nil {
+		t.Fatalf("could not login with user lock_user1: %v", err)
+	}
+
+	cdn := getCDNName(t)
+
+	// Currently, no user has a lock on the "bar" CDN, so when "lock_user1", which does not have the lock on CDN "bar", tries to snap it, this should pass
+	opts := client.NewRequestOptions()
+	opts.QueryParameters.Set("cdn", cdn)
+	_, _, err = userSession.SnapshotCRConfig(opts)
+	if err != nil {
+		t.Errorf("expected no error while snapping cdn %s by user %s, but got %v", cdn, *user1.Username, err)
+	}
+
+	// Create a lock for this user
+	_, _, err = userSession.CreateCDNLock(tc.CDNLock{
+		CDN:     cdn,
+		Message: util.StrPtr("test lock"),
+		Soft:    util.BoolPtr(true),
+	}, client.RequestOptions{})
+	if err != nil {
+		t.Fatalf("couldn't create cdn lock: %v", err)
+	}
+
+	// "lock_user1", which has the lock on CDN "bar", tries to snap it -> this should pass
+	_, _, err = userSession.SnapshotCRConfig(opts)
+	if err != nil {
+		t.Errorf("expected no error while snapping cdn %s by user %s, but got %v", cdn, *user1.Username, err)
+	}
+
+	// Admin user, which doesn't have the lock on the CDN "bar", is trying to snap it -> this should fail
+	_, reqInf, err := TOSession.SnapshotCRConfig(opts)
+	if err == nil {
+		t.Errorf("expected error while snapping cdn %s by user admin, but got nothing", cdn)
+	}
+	if reqInf.StatusCode != http.StatusForbidden {
+		t.Fatalf("expected a 403 status code, but got %d instead", reqInf.StatusCode)
+	}
+
+	// Delete the lock
+	_, _, err = userSession.DeleteCDNLocks(client.RequestOptions{QueryParameters: url.Values{"cdn": []string{cdn}}})
+	if err != nil {
+		t.Fatalf("expected no error while deleting other user's lock using admin endpoint, but got %v", err)

Review comment:
       I don't think this needs to be fatal; it isn't doing anything after this anyway

##########
File path: traffic_ops/testing/api/v4/cdn_locks_test.go
##########
@@ -45,6 +49,63 @@ func getCDNName(t *testing.T) string {
 	return cdnResp.Response[0].Name
 }
 
+func getCDNNameAndServerID(t *testing.T) (string, int) {
+	serverID := -1
+	cdnResp, _, err := TOSession.GetCDNs(client.RequestOptions{})
+	if err != nil {
+		t.Fatalf("couldn't get CDNs: %v", err)
+	}
+	if len(cdnResp.Response) < 1 {
+		t.Fatalf("no valid CDNs in response")
+	}
+	for _, cdn := range cdnResp.Response {
+		opts := client.NewRequestOptions()
+		opts.QueryParameters.Set("cdn", strconv.Itoa(cdn.ID))
+		serversResp, _, err := TOSession.GetServers(opts)
+		if err != nil {
+			t.Errorf("could not get servers for cdn %s: %v", cdn.Name, err)
+		}
+		if len(serversResp.Response) != 0 {
+			serverID = *serversResp.Response[0].ID
+			return cdn.Name, serverID
+		}
+	}
+	return "", serverID
+}
+
+func getCDNDetailsAndTopologyName(t *testing.T) (int, string, string) {
+	opts := client.NewRequestOptions()
+	topologiesResp, _, err := TOSession.GetTopologies(client.RequestOptions{})
+	if err != nil {
+		t.Fatalf("couldn't get topologies, err: %v", err)
+	}
+	if len(topologiesResp.Response) == 0 {
+		t.Fatal("no topologies returned")
+	}
+	for _, top := range topologiesResp.Response {
+		for _, node := range top.Nodes {
+			opts.QueryParameters.Set("name", node.Cachegroup)
+			cacheGroupResp, _, err := TOSession.GetCacheGroups(opts)
+			if err != nil {
+				t.Errorf("error while GETting cachegroups: %v", err)
+			}
+			if len(cacheGroupResp.Response) != 0 && cacheGroupResp.Response[0].ID != nil {
+				cacheGroupID := *cacheGroupResp.Response[0].ID
+				opts.QueryParameters.Del("name")
+				opts.QueryParameters.Set("cachegroup", strconv.Itoa(cacheGroupID))
+				serversResp, _, err := TOSession.GetServers(opts)
+				if err != nil {
+					t.Errorf("couldn't get servers: %v", err)
+				}
+				if len(serversResp.Response) != 0 && serversResp.Response[0].CDNName != nil && serversResp.Response[0].CDNID != nil {
+					return *serversResp.Response[0].CDNID, *serversResp.Response[0].CDNName, top.Name
+				}
+			}
+		}
+	}
+	return -1, "", ""

Review comment:
       should this mark the test as failed?

##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -101,6 +101,31 @@ const getUserByEmailQuery = getUserBaseQuery + `
 WHERE tm_user.email = $1
 `
 
+// CheckIfCurrentUserHasCdnLock checks if the current user has the lock on the cdn that the requested operation is to be performed on.
+// This will succeed if the either there is no lock by any user on the CDN, or if the current user has the lock on the CDN.
+func CheckIfCurrentUserHasCdnLock(tx *sql.Tx, cdn, user string) (error, error, int) {
+	query := `SELECT username FROM cdn_lock WHERE cdn=$1`
+	var userName string
+	rows, err := tx.Query(query, cdn)
+	if err != nil {
+		if err == sql.ErrNoRows {

Review comment:
       checking for specific errors should use `errors.Is` to unwrap things

##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -534,6 +559,15 @@ func GetCDNNameFromID(tx *sql.Tx, id int64) (tc.CDNName, bool, error) {
 	return tc.CDNName(name), true, nil
 }
 
+// GetCDNNameFromServerID gets the CDN name for the server with the given ID
+func GetCDNNameFromServerID(tx *sql.Tx, serverId int64) (tc.CDNName, error) {
+	name := ""
+	if err := tx.QueryRow(`SELECT name FROM cdn WHERE id = (SELECT cdn_id FROM server WHERE id=$1)`, serverId).Scan(&name); err != nil {
+		return "", errors.New("querying CDN name from server ID : " + err.Error())

Review comment:
       if you use `fmt.Errorf("querying CDN name from server ID: %w", err)` instead will wrap the actual error, that way you can still add context, but if the caller wants to they can check if the returned error is a specific kind of error for specific handling e.g.
   ```go
   cdn, err := dbhelpers.GetCDNNameFromServerID(tx, serverID)
   if err != nil {
   	if errors.Is(err, sql.ErrNoRows) {
   		// handle this with maybe a 404 response or something
   	} else {
   		// more generic error handling
   	}
   }
   ```

##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -534,6 +559,15 @@ func GetCDNNameFromID(tx *sql.Tx, id int64) (tc.CDNName, bool, error) {
 	return tc.CDNName(name), true, nil
 }
 
+// GetCDNNameFromServerID gets the CDN name for the server with the given ID
+func GetCDNNameFromServerID(tx *sql.Tx, serverId int64) (tc.CDNName, error) {
+	name := ""
+	if err := tx.QueryRow(`SELECT name FROM cdn WHERE id = (SELECT cdn_id FROM server WHERE id=$1)`, serverId).Scan(&name); err != nil {
+		return "", errors.New("querying CDN name from server ID : " + err.Error())

Review comment:
       if you use `fmt.Errorf("querying CDN name from server ID: %w", err)` instead, that'll wrap the actual error, that way you can still add context, but if the caller wants to they can check if the returned error is a specific kind of error for specific handling e.g.
   ```go
   cdn, err := dbhelpers.GetCDNNameFromServerID(tx, serverID)
   if err != nil {
   	if errors.Is(err, sql.ErrNoRows) {
   		// handle this with maybe a 404 response or something
   	} else {
   		// more generic error handling
   	}
   }
   ```




-- 
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