From 1387fec51d75506d331a7613008884314c49b564 Mon Sep 17 00:00:00 2001 From: Will Browne Date: Mon, 27 Mar 2023 17:44:06 +0100 Subject: [PATCH] Plugins: Markdown fetch retry with lowercase (#65384) * retry with lowercase * undo incorrect err check * re-add defer to close file * fix test --- pkg/api/plugins.go | 26 ++++-- pkg/api/plugins_test.go | 130 +++++++++++++++++++++++++++++ pkg/plugins/manager/fakes/fakes.go | 7 +- 3 files changed, 155 insertions(+), 8 deletions(-) diff --git a/pkg/api/plugins.go b/pkg/api/plugins.go index 0bf5e17fd3e..3cf46428fe8 100644 --- a/pkg/api/plugins.go +++ b/pkg/api/plugins.go @@ -43,6 +43,8 @@ var pluginsCDNFallbackRedirectRequests = promauto.NewCounterVec(prometheus.Count Help: "Number of requests to the plugins CDN backend redirect fallback handler.", }, []string{"plugin_id", "plugin_version"}) +var ErrUnexpectedFileExtension = errors.New("unexpected file extension") + func (hs *HTTPServer) GetPluginList(c *contextmodel.ReqContext) response.Response { typeFilter := c.Query("type") enabledFilter := c.Query("enabled") @@ -539,12 +541,17 @@ func translatePluginRequestErrorToAPIError(err error) response.Response { func (hs *HTTPServer) pluginMarkdown(ctx context.Context, pluginId string, name string) ([]byte, error) { plugin, exists := hs.pluginStore.Plugin(ctx, pluginId) if !exists { - return nil, plugins.NotFoundError{PluginID: pluginId} + return make([]byte, 0), plugins.NotFoundError{PluginID: pluginId} + } + + file, err := mdFilepath(strings.ToUpper(name)) + if err != nil { + return make([]byte, 0), err } - md, err := plugin.File(mdFilepath(strings.ToUpper(name))) + md, err := plugin.File(file) if err != nil { - md, err = plugin.File(mdFilepath(strings.ToUpper(name))) + md, err = plugin.File(strings.ToLower(file)) if err != nil { return make([]byte, 0), nil } @@ -554,7 +561,6 @@ func (hs *HTTPServer) pluginMarkdown(ctx context.Context, pluginId string, name hs.log.Error("Failed to close plugin markdown file", "err", err) } }() - d, err := io.ReadAll(md) if err != nil { return make([]byte, 0), nil @@ -562,6 +568,14 @@ func (hs *HTTPServer) pluginMarkdown(ctx context.Context, pluginId string, name return d, nil } -func mdFilepath(mdFilename string) string { - return filepath.Clean(filepath.Join("/", fmt.Sprintf("%s.md", mdFilename))) +func mdFilepath(mdFilename string) (string, error) { + fileExt := filepath.Ext(mdFilename) + switch fileExt { + case "md": + return util.CleanRelativePath(mdFilename) + case "": + return util.CleanRelativePath(fmt.Sprintf("%s.md", mdFilename)) + default: + return "", ErrUnexpectedFileExtension + } } diff --git a/pkg/api/plugins_test.go b/pkg/api/plugins_test.go index 26079123ac7..a88485bd87c 100644 --- a/pkg/api/plugins_test.go +++ b/pkg/api/plugins_test.go @@ -1,10 +1,13 @@ package api import ( + "bytes" "context" "encoding/json" + "errors" "fmt" "io" + "io/fs" "net/http" "net/http/httptest" "os" @@ -22,6 +25,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/plugins/config" + "github.com/grafana/grafana/pkg/plugins/manager/fakes" "github.com/grafana/grafana/pkg/plugins/pluginscdn" ac "github.com/grafana/grafana/pkg/services/accesscontrol" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" @@ -464,6 +468,114 @@ func TestMakePluginResourceRequestContentTypeEmpty(t *testing.T) { require.Zero(t, resp.Header().Get("Content-Type")) } +func TestPluginMarkdown(t *testing.T) { + t.Run("Plugin not installed returns error", func(t *testing.T) { + hs := HTTPServer{ + pluginStore: &plugins.FakePluginStore{ + PluginList: []plugins.PluginDTO{}, + }, + } + + pluginID := "test-datasource" + md, err := hs.pluginMarkdown(context.Background(), pluginID, "test") + require.ErrorAs(t, err, &plugins.NotFoundError{PluginID: pluginID}) + require.Equal(t, []byte{}, md) + }) + + t.Run("File fetch will be retried using different casing if error occurs", func(t *testing.T) { + var requestedFiles []string + pluginFiles := &fakes.FakePluginFiles{ + OpenFunc: func(name string) (fs.File, error) { + requestedFiles = append(requestedFiles, name) + return nil, errors.New("some error") + }, + } + + p := createPluginDTO(plugins.JSONData{ID: "test-app"}, plugins.External, pluginFiles) + + hs := HTTPServer{ + pluginStore: &plugins.FakePluginStore{PluginList: []plugins.PluginDTO{p}}, + } + + md, err := hs.pluginMarkdown(context.Background(), p.ID, "reAdMe") + require.NoError(t, err) + require.Equal(t, []byte{}, md) + require.Equal(t, []string{"README.md", "readme.md"}, requestedFiles) + }) + + t.Run("File fetch receive cleaned file paths", func(t *testing.T) { + tcs := []struct { + filePath string + expected []string + }{ + { + filePath: "../../docs", + expected: []string{"DOCS.md"}, + }, + { + filePath: "/../../docs/../docs", + expected: []string{"DOCS.md"}, + }, + { + filePath: "readme.md/../../secrets", + expected: []string{"SECRETS.md"}, + }, + } + + for _, tc := range tcs { + var requestedFiles []string + pluginFiles := &fakes.FakePluginFiles{ + OpenFunc: func(name string) (fs.File, error) { + requestedFiles = append(requestedFiles, name) + return &FakeFile{data: bytes.NewReader(nil)}, nil + }, + } + + p := createPluginDTO(plugins.JSONData{ID: "test-app"}, plugins.External, pluginFiles) + + hs := HTTPServer{ + pluginStore: &plugins.FakePluginStore{PluginList: []plugins.PluginDTO{p}}, + } + + md, err := hs.pluginMarkdown(context.Background(), p.ID, tc.filePath) + require.NoError(t, err) + require.Equal(t, []byte{}, md) + require.Equal(t, tc.expected, requestedFiles) + } + }) + + t.Run("Non markdown file request returns an error", func(t *testing.T) { + p := createPluginDTO(plugins.JSONData{ID: "test-app"}, plugins.External, nil) + hs := HTTPServer{ + pluginStore: &plugins.FakePluginStore{PluginList: []plugins.PluginDTO{p}}, + } + + md, err := hs.pluginMarkdown(context.Background(), p.ID, "test.json") + require.ErrorIs(t, err, ErrUnexpectedFileExtension) + require.Equal(t, []byte{}, md) + }) + + t.Run("Happy path", func(t *testing.T) { + data := []byte{1, 2, 3} + fakeFile := &FakeFile{data: bytes.NewReader(data)} + + pluginFiles := &fakes.FakePluginFiles{ + OpenFunc: func(name string) (fs.File, error) { + return fakeFile, nil + }, + } + + p := createPluginDTO(plugins.JSONData{ID: "test-app"}, plugins.External, pluginFiles) + hs := HTTPServer{ + pluginStore: &plugins.FakePluginStore{PluginList: []plugins.PluginDTO{p}}, + } + + md, err := hs.pluginMarkdown(context.Background(), p.ID, "someFile") + require.NoError(t, err) + require.Equal(t, data, md) + }) +} + func callGetPluginAsset(sc *scenarioContext) { sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() } @@ -607,3 +719,21 @@ func createPluginDTO(jd plugins.JSONData, class plugins.Class, files plugins.FS) return p.ToDTO() } + +var _ fs.File = (*FakeFile)(nil) + +type FakeFile struct { + data io.Reader +} + +func (f *FakeFile) Stat() (fs.FileInfo, error) { + return nil, nil +} + +func (f *FakeFile) Read(bytes []byte) (int, error) { + return f.data.Read(bytes) +} + +func (f *FakeFile) Close() error { + return nil +} diff --git a/pkg/plugins/manager/fakes/fakes.go b/pkg/plugins/manager/fakes/fakes.go index a10a36bedc7..a4a9ad1a4d0 100644 --- a/pkg/plugins/manager/fakes/fakes.go +++ b/pkg/plugins/manager/fakes/fakes.go @@ -358,7 +358,7 @@ func (f *FakeRoleRegistry) DeclarePluginRoles(_ context.Context, _ string, _ str } type FakePluginFiles struct { - FS fs.FS + OpenFunc func(name string) (fs.File, error) base string } @@ -370,7 +370,10 @@ func NewFakePluginFiles(base string) *FakePluginFiles { } func (f *FakePluginFiles) Open(name string) (fs.File, error) { - return f.FS.Open(name) + if f.OpenFunc != nil { + return f.OpenFunc(name) + } + return nil, nil } func (f *FakePluginFiles) Base() string {