grafana-cli: Fix file path processing, returning of errors (#26954)

* grafana-cli: Fix file path processing, returning of errors

pkg/cmd/grafana-cli/commands/install_command.go

@@ -8,7 +8,6 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"path"
 	"path/filepath"
 	"regexp"
 	"runtime"
@@ -205,9 +204,10 @@ func SelectVersion(plugin *models.Plugin, version string) (*models.Version, erro
 	return &ver, nil
 }
 
-func RemoveGitBuildFromName(pluginName, filename string) string {
+var reGitBuild = regexp.MustCompile("^[a-zA-Z0-9_.-]*/")
-	r := regexp.MustCompile("^[a-zA-Z0-9_.-]*/")
+
-	return r.ReplaceAllString(filename, pluginName+"/")
+func removeGitBuildFromName(pluginName, filename string) string {
+	return reGitBuild.ReplaceAllString(filename, pluginName+"/")
 }
 
 const permissionsDeniedMessage = "could not create %q, permission denied, make sure you have write access to plugin dir"
@@ -220,43 +220,46 @@ func extractFiles(archiveFile string, pluginName string, filePath string, allowS
 		return err
 	}
 	for _, zf := range r.File {
-		newFileName := RemoveGitBuildFromName(pluginName, zf.Name)
+		newFileName := removeGitBuildFromName(pluginName, zf.Name)
 		if !isPathSafe(newFileName, filepath.Join(filePath, pluginName)) {
 			return fmt.Errorf("filepath: %q tries to write outside of plugin directory: %q, this can be a security risk",
 				zf.Name, filepath.Join(filePath, pluginName))
 		}
-		newFile := path.Join(filePath, newFileName)
+		newFile := filepath.Join(filePath, newFileName)
 
 		if zf.FileInfo().IsDir() {
-			err := os.Mkdir(newFile, 0755)
+			if err := os.MkdirAll(newFile, 0755); err != nil {
 				if os.IsPermission(err) {
 					return fmt.Errorf(permissionsDeniedMessage, newFile)
 				}
-		} else {
+
+				return err
+			}
+
+			continue
+		}
+
 		// Create needed directories to extract file
-			err := os.MkdirAll(filepath.Dir(newFile), 0755)
+		if err := os.MkdirAll(filepath.Dir(newFile), 0755); err != nil {
-			if err != nil {
 			return errutil.Wrap("failed to create directory to extract plugin files", err)
 		}
 
 		if isSymlink(zf) {
 			if !allowSymlinks {
-					logger.Errorf("%v: plugin archive contains symlink which is not allowed. Skipping \n", zf.Name)
+				logger.Warnf("%v: plugin archive contains a symlink, which is not allowed. Skipping \n", zf.Name)
 				continue
 			}
-				err = extractSymlink(zf, newFile)
+			if err := extractSymlink(zf, newFile); err != nil {
-				if err != nil {
 				logger.Errorf("Failed to extract symlink: %v \n", err)
 				continue
 			}
-			} else {
+			continue
-				err = extractFile(zf, newFile)
+		}
-				if err != nil {
+
+		if err := extractFile(zf, newFile); err != nil {
 			return errutil.Wrap("failed to extract file", err)
 		}
 	}
-		}
-	}
 
 	return nil
 }
@@ -272,12 +275,10 @@ func extractSymlink(file *zip.File, filePath string) error {
 		return errutil.Wrap("failed to extract file", err)
 	}
 	buf := new(bytes.Buffer)
-	_, err = io.Copy(buf, src)
+	if _, err := io.Copy(buf, src); err != nil {
-	if err != nil {
 		return errutil.Wrap("failed to copy symlink contents", err)
 	}
-	err = os.Symlink(strings.TrimSpace(buf.String()), filePath)
+	if err := os.Symlink(strings.TrimSpace(buf.String()), filePath); err != nil {
-	if err != nil {
 		return errutil.Wrapf(err, "failed to make symbolic link for %v", filePath)
 	}
 	return nil

pkg/cmd/grafana-cli/commands/install_command_test.go

@@ -4,46 +4,30 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path/filepath"
 	"runtime"
 	"testing"
 
 	"github.com/grafana/grafana/pkg/cmd/grafana-cli/commands/commandstest"
 	"github.com/grafana/grafana/pkg/cmd/grafana-cli/models"
-	. "github.com/smartystreets/goconvey/convey"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func TestFolderNameReplacement(t *testing.T) {
+func TestRemoveGitBuildFromName(t *testing.T) {
-	Convey("path containing git commit path", t, func() {
+	pluginName := "datasource-kairosdb"
-		pluginName := "datasource-plugin-kairosdb"
 
+	// The root directory should get renamed to the plugin name
 	paths := map[string]string{
-			"datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/":                     "datasource-plugin-kairosdb/",
+		"datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/":                     "datasource-kairosdb/",
-			"datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/README.md":            "datasource-plugin-kairosdb/README.md",
+		"datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/README.md":            "datasource-kairosdb/README.md",
-			"datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/partials/":            "datasource-plugin-kairosdb/partials/",
+		"datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/partials/":            "datasource-kairosdb/partials/",
-			"datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/partials/config.html": "datasource-plugin-kairosdb/partials/config.html",
+		"datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/partials/config.html": "datasource-kairosdb/partials/config.html",
 	}
-
+	for pth, exp := range paths {
-		Convey("should be replaced with plugin name", func() {
+		name := removeGitBuildFromName(pluginName, pth)
-			for k, v := range paths {
+		assert.Equal(t, exp, name)
-				So(RemoveGitBuildFromName(pluginName, k), ShouldEqual, v)
 	}
-		})
-	})
-
-	Convey("path containing git commit path", t, func() {
-		pluginName := "app-example"
-		paths := map[string]string{
-			"app-plugin-example-3c28f65ac6fb7f1e234b0364b97081d836495439/": "app-example/",
-		}
-
-		Convey("should be replaced with plugin name", func() {
-			for k, v := range paths {
-				So(RemoveGitBuildFromName(pluginName, k), ShouldEqual, v)
-			}
-		})
-	})
 }
 
 func TestExtractFiles(t *testing.T) {
@@ -52,28 +36,29 @@ func TestExtractFiles(t *testing.T) {
 		pluginDir, del := setupFakePluginsDir(t)
 		defer del()
 
-		archive := "testdata/grafana-simple-json-datasource-ec18fa4da8096a952608a7e4c7782b4260b41bcf.zip"
+		archive := filepath.Join("testdata", "grafana-simple-json-datasource-ec18fa4da8096a952608a7e4c7782b4260b41bcf.zip")
 		err := extractFiles(archive, "grafana-simple-json-datasource", pluginDir, false)
-		assert.Nil(t, err)
+		require.NoError(t, err)
 
 		//File in zip has permissions 755
-		fileInfo, err := os.Stat(pluginDir + "/grafana-simple-json-datasource/simple-plugin_darwin_amd64")
+		fileInfo, err := os.Stat(filepath.Join(pluginDir, "grafana-simple-json-datasource",
-		assert.Nil(t, err)
+			"simple-plugin_darwin_amd64"))
+		require.NoError(t, err)
 		assert.Equal(t, "-rwxr-xr-x", fileInfo.Mode().String())
 
 		//File in zip has permission 755
 		fileInfo, err = os.Stat(pluginDir + "/grafana-simple-json-datasource/simple-plugin_linux_amd64")
-		assert.Nil(t, err)
+		require.NoError(t, err)
 		assert.Equal(t, "-rwxr-xr-x", fileInfo.Mode().String())
 
 		//File in zip has permission 644
 		fileInfo, err = os.Stat(pluginDir + "/grafana-simple-json-datasource/simple-plugin_windows_amd64.exe")
-		assert.Nil(t, err)
+		require.NoError(t, err)
 		assert.Equal(t, "-rw-r--r--", fileInfo.Mode().String())
 
 		//File in zip has permission 755
 		fileInfo, err = os.Stat(pluginDir + "/grafana-simple-json-datasource/non-plugin-binary")
-		assert.Nil(t, err)
+		require.NoError(t, err)
 		assert.Equal(t, "-rwxr-xr-x", fileInfo.Mode().String())
 	})
 
@@ -82,10 +67,10 @@ func TestExtractFiles(t *testing.T) {
 		defer del()
 
 		err := extractFiles("testdata/plugin-with-symlink.zip", "plugin-with-symlink", pluginDir, false)
-		assert.Nil(t, err)
+		require.NoError(t, err)
 
 		_, err = os.Stat(pluginDir + "/plugin-with-symlink/text.txt")
-		assert.Nil(t, err)
+		require.NoError(t, err)
 		_, err = os.Stat(pluginDir + "/plugin-with-symlink/symlink_to_txt")
 		assert.NotNil(t, err)
 	})
@@ -96,10 +81,10 @@ func TestExtractFiles(t *testing.T) {
 		defer del()
 
 		err := extractFiles("testdata/plugin-with-symlink.zip", "plugin-with-symlink", pluginDir, true)
-		assert.Nil(t, err)
+		require.NoError(t, err)
 
 		_, err = os.Stat(pluginDir + "/plugin-with-symlink/symlink_to_txt")
-		assert.Nil(t, err)
+		require.NoError(t, err)
 		fmt.Println(err)
 	})
 }
@@ -199,7 +184,7 @@ func TestSelectVersion(t *testing.T) {
 			),
 			"",
 		)
-		assert.Nil(t, err)
+		require.NoError(t, err)
 		assert.Equal(t, "1.0.0", ver.Version)
 	})
 
@@ -208,7 +193,7 @@ func TestSelectVersion(t *testing.T) {
 			makePluginWithVersions(versionArg{Version: "2.0.0"}, versionArg{Version: "1.0.0"}),
 			"",
 		)
-		assert.Nil(t, err)
+		require.NoError(t, err)
 		assert.Equal(t, "2.0.0", ver.Version)
 	})
 
@@ -220,7 +205,7 @@ func TestSelectVersion(t *testing.T) {
 			),
 			"1.0.0",
 		)
-		assert.Nil(t, err)
+		require.NoError(t, err)
 		assert.Equal(t, "1.0.0", ver.Version)
 	})
 }
@@ -234,8 +219,8 @@ func setupFakePluginsDir(t *testing.T) (string, func()) {
 	require.Nil(t, err)
 
 	return dirname, func() {
-		err = os.RemoveAll(dirname)
+		err := os.RemoveAll(dirname)
-		assert.Nil(t, err)
+		require.NoError(t, err)
 	}
 }
 

pkg/cmd/grafana-cli/services/api_client.go

@@ -146,7 +146,11 @@ func sendRequest(client http.Client, repoUrl string, subPaths ...string) (io.Rea
 }
 
 func createRequest(repoUrl string, subPaths ...string) (*http.Request, error) {
-	u, _ := url.Parse(repoUrl)
+	u, err := url.Parse(repoUrl)
+	if err != nil {
+		return nil, err
+	}
+
 	for _, v := range subPaths {
 		u.Path = path.Join(u.Path, v)
 	}
@@ -170,7 +174,7 @@ func handleResponse(res *http.Response) (io.ReadCloser, error) {
 	}
 
 	if res.StatusCode/100 != 2 && res.StatusCode/100 != 4 {
-		return nil, fmt.Errorf("Api returned invalid status: %s", res.Status)
+		return nil, fmt.Errorf("API returned invalid status: %s", res.Status)
 	}
 
 	if res.StatusCode/100 == 4 {

pkg/cmd/grafana-cli/services/services.go

@@ -7,7 +7,7 @@ import (
 	"fmt"
 	"net"
 	"net/http"
-	"path"
+	"path/filepath"
 	"time"
 
 	"github.com/grafana/grafana/pkg/cmd/grafana-cli/logger"
@@ -64,16 +64,12 @@ func makeHttpClient(skipTLSVerify bool, timeout time.Duration) http.Client {
 }
 
 func ReadPlugin(pluginDir, pluginName string) (models.InstalledPlugin, error) {
-	distPluginDataPath := path.Join(pluginDir, pluginName, "dist", "plugin.json")
+	distPluginDataPath := filepath.Join(pluginDir, pluginName, "dist", "plugin.json")
-
-	var data []byte
-	var err error
-	data, err = IoHelper.ReadFile(distPluginDataPath)
 
+	data, err := IoHelper.ReadFile(distPluginDataPath)
 	if err != nil {
-		pluginDataPath := path.Join(pluginDir, pluginName, "plugin.json")
+		pluginDataPath := filepath.Join(pluginDir, pluginName, "plugin.json")
 		data, err = IoHelper.ReadFile(pluginDataPath)
-
 		if err != nil {
 			return models.InstalledPlugin{}, errors.New("Could not find dist/plugin.json or plugin.json on  " + pluginName + " in " + pluginDir)
 		}
@@ -110,7 +106,7 @@ func GetLocalPlugins(pluginDir string) []models.InstalledPlugin {
 
 func RemoveInstalledPlugin(pluginPath, pluginName string) error {
 	logger.Infof("Removing plugin: %v\n", pluginName)
-	pluginDir := path.Join(pluginPath, pluginName)
+	pluginDir := filepath.Join(pluginPath, pluginName)
 
 	_, err := IoHelper.Stat(pluginDir)
 	if err != nil {

pkg/cmd/grafana-cli/utils/grafana_path.go

@@ -2,7 +2,6 @@ package utils
 
 import (
 	"os"
-	"path"
 	"path/filepath"
 
 	"github.com/grafana/grafana/pkg/cmd/grafana-cli/logger"
@@ -26,7 +25,7 @@ func getGrafanaRoot() (string, error) {
 		return "", errutil.Wrap("failed to get executable path", err)
 	}
 	exPath := filepath.Dir(ex)
-	_, last := path.Split(exPath)
+	_, last := filepath.Split(exPath)
 	if last == "bin" {
 		// In dev env the executable for current platform is created in 'bin/' dir
 		return filepath.Join(exPath, ".."), nil