From c8154b9fe219e020c17b27103d6805e977bddaf3 Mon Sep 17 00:00:00 2001 From: Jeff Levin Date: Fri, 21 Jan 2022 14:22:43 -0900 Subject: [PATCH] Datasource API: #32556 resolve readonly datasources can be modified (#44186) * Check if datasource is read-only when making an update * Standardize api returning a 404 if datasource is not found while making an update Co-authored-by: Marcus Efraimsson , Jesse Weaver --- pkg/api/datasources.go | 17 ++++++++++ pkg/api/datasources_test.go | 62 +++++++++++++++++++++++++++++++++++-- 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/pkg/api/datasources.go b/pkg/api/datasources.go index 28989cb174e..2f2acf54175 100644 --- a/pkg/api/datasources.go +++ b/pkg/api/datasources.go @@ -87,6 +87,7 @@ func (hs *HTTPServer) getDataSourceAccessControlMetadata(c *models.ReqContext, d return accesscontrol.GetResourcesMetadata(c.Req.Context(), userPermissions, "datasources", dsIDs)[key], nil } +// GET /api/datasources/:id func (hs *HTTPServer) GetDataSourceById(c *models.ReqContext) response.Response { id, err := strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) if err != nil { @@ -123,6 +124,7 @@ func (hs *HTTPServer) GetDataSourceById(c *models.ReqContext) response.Response return response.JSON(200, &dto) } +// DELETE /api/datasources/:id func (hs *HTTPServer) DeleteDataSourceById(c *models.ReqContext) response.Response { id, err := strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) if err != nil { @@ -220,6 +222,7 @@ func (hs *HTTPServer) DeleteDataSourceByUID(c *models.ReqContext) response.Respo }) } +// DELETE /api/datasources/name/:name func (hs *HTTPServer) DeleteDataSourceByName(c *models.ReqContext) response.Response { name := web.Params(c.Req)[":name"] @@ -265,6 +268,7 @@ func validateURL(tp string, u string) response.Response { return nil } +// POST /api/datasources/ func AddDataSource(c *models.ReqContext) response.Response { cmd := models.AddDataSourceCommand{} if err := web.Bind(c.Req, &cmd); err != nil { @@ -293,6 +297,7 @@ func AddDataSource(c *models.ReqContext) response.Response { }) } +// PUT /api/datasources/:id func (hs *HTTPServer) UpdateDataSource(c *models.ReqContext) response.Response { cmd := models.UpdateDataSourceCommand{} if err := web.Bind(c.Req, &cmd); err != nil { @@ -309,6 +314,18 @@ func (hs *HTTPServer) UpdateDataSource(c *models.ReqContext) response.Response { return resp } + ds, err := getRawDataSourceById(c.Req.Context(), cmd.Id, cmd.OrgId) + if err != nil { + if errors.Is(err, models.ErrDataSourceNotFound) { + return response.Error(404, "Data source not found", nil) + } + return response.Error(500, "Failed to update datasource", err) + } + + if ds.ReadOnly { + return response.Error(403, "Cannot update read-only data source", nil) + } + err = hs.fillWithSecureJSONData(c.Req.Context(), &cmd) if err != nil { return response.Error(500, "Failed to update datasource", err) diff --git a/pkg/api/datasources_test.go b/pkg/api/datasources_test.go index 90ea9a4dccd..ac43d1cd016 100644 --- a/pkg/api/datasources_test.go +++ b/pkg/api/datasources_test.go @@ -12,13 +12,12 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/setting" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - - "github.com/grafana/grafana/pkg/bus" ) const ( @@ -192,6 +191,16 @@ func TestAPI_Datasources_AccessControl(t *testing.T) { Type: "postgresql", Access: "Proxy", } + testDatasourceReadOnly := models.DataSource{ + Id: 4, + Uid: "testUID", + OrgId: testOrgID, + Name: "test", + Url: "http://localhost:5432", + Type: "postgresql", + Access: "Proxy", + ReadOnly: true, + } getDatasourceStub := func(ctx context.Context, query *models.GetDataSourceQuery) error { result := testDatasource result.Id = query.Id @@ -211,6 +220,21 @@ func TestAPI_Datasources_AccessControl(t *testing.T) { cmd.Result = &testDatasource return nil } + updateDatasourceReadOnlyStub := func(ctx context.Context, cmd *models.UpdateDataSourceCommand) error { + cmd.Result = &testDatasourceReadOnly + return nil + } + + getDatasourceNotFoundStub := func(ctx context.Context, cmd *models.GetDataSourceQuery) error { + cmd.Result = nil + return models.ErrDataSourceNotFound + } + + getDatasourceReadOnlyStub := func(ctx context.Context, query *models.GetDataSourceQuery) error { + query.Result = &testDatasourceReadOnly + return nil + } + deleteDatasourceStub := func(ctx context.Context, cmd *models.DeleteDataSourceCommand) error { cmd.DeletedDatasourcesCount = 1 return nil @@ -233,13 +257,28 @@ func TestAPI_Datasources_AccessControl(t *testing.T) { }) return bytes.NewReader(s) } - type acTestCaseWithHandler struct { busStubs []bus.HandlerFunc body func() io.Reader accessControlTestCase } tests := []acTestCaseWithHandler{ + { + busStubs: []bus.HandlerFunc{getDatasourceNotFoundStub, updateDatasourceStub}, + body: updateDatasourceBody, + accessControlTestCase: accessControlTestCase{ + expectedCode: http.StatusNotFound, + desc: "DatasourcesPut should return 404 if datasource not found", + url: fmt.Sprintf("/api/datasources/%v", "12345678"), + method: http.MethodPut, + permissions: []*accesscontrol.Permission{ + { + Action: ActionDatasourcesWrite, + Scope: ScopeDatasourcesAll, + }, + }, + }, + }, { busStubs: []bus.HandlerFunc{getDatasourcesStub}, accessControlTestCase: accessControlTestCase{ @@ -304,6 +343,22 @@ func TestAPI_Datasources_AccessControl(t *testing.T) { permissions: []*accesscontrol.Permission{{Action: "wrong"}}, }, }, + { + busStubs: []bus.HandlerFunc{getDatasourceReadOnlyStub, updateDatasourceReadOnlyStub}, + body: updateDatasourceBody, + accessControlTestCase: accessControlTestCase{ + expectedCode: http.StatusForbidden, + desc: "DatasourcesPut should return 403 for read only datasource", + url: fmt.Sprintf("/api/datasources/%v", testDatasourceReadOnly.Id), + method: http.MethodPut, + permissions: []*accesscontrol.Permission{ + { + Action: ActionDatasourcesWrite, + Scope: fmt.Sprintf("datasources:id:%v", testDatasourceReadOnly.Id), + }, + }, + }, + }, { busStubs: []bus.HandlerFunc{getDatasourceStub, deleteDatasourceStub}, accessControlTestCase: accessControlTestCase{ @@ -505,6 +560,7 @@ func TestAPI_Datasources_AccessControl(t *testing.T) { } else { sc.req, err = http.NewRequest(test.method, test.url, nil) } + assert.NoError(t, err) sc.exec()