From 60496fbae39a6fa41fb4f4564ceae308412665dd Mon Sep 17 00:00:00 2001 From: Kousik Mitra Date: Mon, 17 Jul 2023 19:57:19 +0530 Subject: [PATCH] Chore: Return correct error for name taken and validation error on add/update datasource (#70465) --- pkg/api/datasources.go | 17 ++- pkg/api/datasources_test.go | 39 +++++ pkg/services/datasources/errors.go | 8 +- .../datasources/service/datasource.go | 42 +++++ .../datasources/service/datasource_test.go | 144 ++++++++++++++++++ 5 files changed, 243 insertions(+), 7 deletions(-) diff --git a/pkg/api/datasources.go b/pkg/api/datasources.go index 9d11815792b..2607566bd96 100644 --- a/pkg/api/datasources.go +++ b/pkg/api/datasources.go @@ -390,14 +390,14 @@ func (hs *HTTPServer) AddDataSource(c *contextmodel.ReqContext) response.Respons dataSource, err := hs.DataSourcesService.AddDataSource(c.Req.Context(), &cmd) if err != nil { if errors.Is(err, datasources.ErrDataSourceNameExists) || errors.Is(err, datasources.ErrDataSourceUidExists) { - return response.Error(409, err.Error(), err) + return response.Error(http.StatusConflict, err.Error(), err) } if errors.As(err, &secretsPluginError) { - return response.Error(500, "Failed to add datasource: "+err.Error(), err) + return response.Error(http.StatusInternalServerError, "Failed to add datasource: "+err.Error(), err) } - return response.Error(500, "Failed to add datasource", err) + return response.ErrOrFallback(http.StatusInternalServerError, "Failed to add datasource", err) } // Clear permission cache for the user who's created the data source, so that new permissions are fetched for their next call @@ -512,14 +512,19 @@ func (hs *HTTPServer) updateDataSourceByID(c *contextmodel.ReqContext, ds *datas _, err := hs.DataSourcesService.UpdateDataSource(c.Req.Context(), &cmd) if err != nil { + if errors.Is(err, datasources.ErrDataSourceNameExists) { + return response.Error(http.StatusConflict, "Failed to update datasource: "+err.Error(), err) + } + if errors.Is(err, datasources.ErrDataSourceUpdatingOldVersion) { - return response.Error(409, "Datasource has already been updated by someone else. Please reload and try again", err) + return response.Error(http.StatusConflict, "Datasource has already been updated by someone else. Please reload and try again", err) } if errors.As(err, &secretsPluginError) { - return response.Error(500, "Failed to update datasource: "+err.Error(), err) + return response.Error(http.StatusInternalServerError, "Failed to update datasource: "+err.Error(), err) } - return response.Error(500, "Failed to update datasource", err) + + return response.ErrOrFallback(http.StatusInternalServerError, "Failed to update datasource", err) } query := datasources.GetDataSourceQuery{ diff --git a/pkg/api/datasources_test.go b/pkg/api/datasources_test.go index 543bc688c63..5d4f97b809a 100644 --- a/pkg/api/datasources_test.go +++ b/pkg/api/datasources_test.go @@ -23,6 +23,7 @@ import ( "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/datasources/permissions" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/web" "github.com/grafana/grafana/pkg/web/webtest" ) @@ -248,6 +249,38 @@ func TestUpdateDataSource_URLWithoutProtocol(t *testing.T) { assert.Equal(t, 200, sc.resp.Code) } +// Updating data source name where data source with same name exists. +func TestUpdateDataSourceByID_DataSourceNameExists(t *testing.T) { + hs := &HTTPServer{ + DataSourcesService: &dataSourcesServiceMock{ + expectedDatasource: &datasources.DataSource{}, + mockUpdateDataSource: func(ctx context.Context, cmd *datasources.UpdateDataSourceCommand) (*datasources.DataSource, error) { + return nil, datasources.ErrDataSourceNameExists + }, + }, + Cfg: setting.NewCfg(), + AccessControl: acimpl.ProvideAccessControl(setting.NewCfg()), + accesscontrolService: actest.FakeService{}, + Live: newTestLive(t, nil), + } + + sc := setupScenarioContext(t, "/api/datasources/1") + + sc.m.Put(sc.url, routing.Wrap(func(c *contextmodel.ReqContext) response.Response { + c.Req = web.SetURLParams(c.Req, map[string]string{":id": "1"}) + c.Req.Body = mockRequestBody(datasources.UpdateDataSourceCommand{ + Access: "direct", + Type: "test", + Name: "test", + }) + return hs.UpdateDataSourceByID(c) + })) + + sc.fakeReqWithParams("PUT", sc.url, map[string]string{}).exec() + + require.Equal(t, http.StatusConflict, sc.resp.Code) +} + func TestAPI_datasources_AccessControl(t *testing.T) { type testCase struct { desc string @@ -359,6 +392,8 @@ type dataSourcesServiceMock struct { expectedDatasources []*datasources.DataSource expectedDatasource *datasources.DataSource expectedError error + + mockUpdateDataSource func(ctx context.Context, cmd *datasources.UpdateDataSourceCommand) (*datasources.DataSource, error) } func (m *dataSourcesServiceMock) GetDataSource(ctx context.Context, query *datasources.GetDataSourceQuery) (*datasources.DataSource, error) { @@ -386,6 +421,10 @@ func (m *dataSourcesServiceMock) AddDataSource(ctx context.Context, cmd *datasou } func (m *dataSourcesServiceMock) UpdateDataSource(ctx context.Context, cmd *datasources.UpdateDataSourceCommand) (*datasources.DataSource, error) { + if m.mockUpdateDataSource != nil { + return m.mockUpdateDataSource(ctx, cmd) + } + return m.expectedDatasource, m.expectedError } diff --git a/pkg/services/datasources/errors.go b/pkg/services/datasources/errors.go index b220f93982d..b6e8759cddc 100644 --- a/pkg/services/datasources/errors.go +++ b/pkg/services/datasources/errors.go @@ -1,6 +1,10 @@ package datasources -import "errors" +import ( + "errors" + + "github.com/grafana/grafana/pkg/util/errutil" +) var ( ErrDataSourceNotFound = errors.New("data source not found") @@ -11,4 +15,6 @@ var ( ErrDataSourceFailedGenerateUniqueUid = errors.New("failed to generate unique datasource ID") ErrDataSourceIdentifierNotSet = errors.New("unique identifier and org id are needed to be able to get or delete a datasource") ErrDatasourceIsReadOnly = errors.New("data source is readonly, can only be updated from configuration") + ErrDataSourceNameInvalid = errutil.NewBase(errutil.StatusValidationFailed, "datasource.nameInvalid", errutil.WithPublicMessage("Invalid datasource name.")) + ErrDataSourceURLInvalid = errutil.NewBase(errutil.StatusValidationFailed, "datasource.urlInvalid", errutil.WithPublicMessage("Invalid datasource url.")) ) diff --git a/pkg/services/datasources/service/datasource.go b/pkg/services/datasources/service/datasource.go index 5188b48c4f3..cdfad67cd63 100644 --- a/pkg/services/datasources/service/datasource.go +++ b/pkg/services/datasources/service/datasource.go @@ -4,6 +4,7 @@ import ( "context" "crypto/tls" "encoding/json" + "errors" "fmt" "net/http" "strconv" @@ -26,6 +27,11 @@ import ( "github.com/grafana/grafana/pkg/setting" ) +const ( + maxDatasourceNameLen = 190 + maxDatasourceUrlLen = 255 +) + type Service struct { SQLStore Store SecretsStore kvstore.SecretsKVStore @@ -172,6 +178,10 @@ func (s *Service) GetDataSourcesByType(ctx context.Context, query *datasources.G func (s *Service) AddDataSource(ctx context.Context, cmd *datasources.AddDataSourceCommand) (*datasources.DataSource, error) { var dataSource *datasources.DataSource + if err := validateFields(cmd.Name, cmd.URL); err != nil { + return dataSource, err + } + return dataSource, s.db.InTransaction(ctx, func(ctx context.Context) error { var err error @@ -231,6 +241,11 @@ func (s *Service) DeleteDataSource(ctx context.Context, cmd *datasources.DeleteD func (s *Service) UpdateDataSource(ctx context.Context, cmd *datasources.UpdateDataSourceCommand) (*datasources.DataSource, error) { var dataSource *datasources.DataSource + + if err := validateFields(cmd.Name, cmd.URL); err != nil { + return dataSource, err + } + return dataSource, s.db.InTransaction(ctx, func(ctx context.Context) error { var err error @@ -243,6 +258,21 @@ func (s *Service) UpdateDataSource(ctx context.Context, cmd *datasources.UpdateD return err } + if cmd.Name != "" && cmd.Name != dataSource.Name { + query := &datasources.GetDataSourceQuery{ + Name: cmd.Name, + OrgID: cmd.OrgID, + } + exist, err := s.SQLStore.GetDataSource(ctx, query) + if exist != nil { + return datasources.ErrDataSourceNameExists + } + + if err != nil && !errors.Is(err, datasources.ErrDataSourceNotFound) { + return err + } + } + err = s.fillWithSecureJSONData(ctx, cmd, dataSource) if err != nil { return err @@ -631,6 +661,18 @@ func (s *Service) fillWithSecureJSONData(ctx context.Context, cmd *datasources.U return nil } +func validateFields(name, url string) error { + if len(name) > maxDatasourceNameLen { + return datasources.ErrDataSourceNameInvalid.Errorf("max length is %d", maxDatasourceNameLen) + } + + if len(url) > maxDatasourceUrlLen { + return datasources.ErrDataSourceURLInvalid.Errorf("max length is %d", maxDatasourceUrlLen) + } + + return nil +} + func readQuotaConfig(cfg *setting.Cfg) (*quota.Map, error) { limits := "a.Map{} diff --git a/pkg/services/datasources/service/datasource_test.go b/pkg/services/datasources/service/datasource_test.go index ccd6cd979e2..e830d2e04f3 100644 --- a/pkg/services/datasources/service/datasource_test.go +++ b/pkg/services/datasources/service/datasource_test.go @@ -9,8 +9,10 @@ import ( "testing" "time" + "github.com/google/uuid" sdkhttpclient "github.com/grafana/grafana-plugin-sdk-go/backend/httpclient" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/components/simplejson" @@ -18,6 +20,7 @@ import ( "github.com/grafana/grafana/pkg/infra/httpclient" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/accesscontrol/actest" acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/featuremgmt" @@ -45,6 +48,147 @@ func (d *dataSourceMockRetriever) GetDataSource(ctx context.Context, query *data return nil, datasources.ErrDataSourceNotFound } +func TestService_AddDataSource(t *testing.T) { + cfg := &setting.Cfg{} + + t.Run("should return validation error if command validation failed", func(t *testing.T) { + sqlStore := db.InitTestDB(t) + secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore()) + secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger")) + quotaService := quotatest.New(false, nil) + mockPermission := acmock.NewMockedPermissionsService() + dsService, err := ProvideService(sqlStore, secretsService, secretsStore, cfg, featuremgmt.WithFeatures(), actest.FakeAccessControl{}, mockPermission, quotaService) + require.NoError(t, err) + + cmd := &datasources.AddDataSourceCommand{ + OrgID: 1, + Name: string(make([]byte, 256)), + } + + _, err = dsService.AddDataSource(context.Background(), cmd) + require.EqualError(t, err, "[datasource.nameInvalid] max length is 190") + + cmd = &datasources.AddDataSourceCommand{ + OrgID: 1, + URL: string(make([]byte, 256)), + } + + _, err = dsService.AddDataSource(context.Background(), cmd) + require.EqualError(t, err, "[datasource.urlInvalid] max length is 255") + }) +} + +func TestService_UpdateDataSource(t *testing.T) { + cfg := &setting.Cfg{} + + t.Run("should return not found error if datasource not found", func(t *testing.T) { + sqlStore := db.InitTestDB(t) + secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore()) + secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger")) + quotaService := quotatest.New(false, nil) + mockPermission := acmock.NewMockedPermissionsService() + dsService, err := ProvideService(sqlStore, secretsService, secretsStore, cfg, featuremgmt.WithFeatures(), actest.FakeAccessControl{}, mockPermission, quotaService) + require.NoError(t, err) + + cmd := &datasources.UpdateDataSourceCommand{ + UID: uuid.New().String(), + ID: 1, + OrgID: 1, + } + + _, err = dsService.UpdateDataSource(context.Background(), cmd) + require.ErrorIs(t, err, datasources.ErrDataSourceNotFound) + }) + + t.Run("should return validation error if command validation failed", func(t *testing.T) { + sqlStore := db.InitTestDB(t) + secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore()) + secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger")) + quotaService := quotatest.New(false, nil) + mockPermission := acmock.NewMockedPermissionsService() + dsService, err := ProvideService(sqlStore, secretsService, secretsStore, cfg, featuremgmt.WithFeatures(), actest.FakeAccessControl{}, mockPermission, quotaService) + require.NoError(t, err) + + cmd := &datasources.UpdateDataSourceCommand{ + ID: 1, + OrgID: 1, + Name: string(make([]byte, 256)), + } + + _, err = dsService.UpdateDataSource(context.Background(), cmd) + require.EqualError(t, err, "[datasource.nameInvalid] max length is 190") + + cmd = &datasources.UpdateDataSourceCommand{ + ID: 1, + OrgID: 1, + URL: string(make([]byte, 256)), + } + + _, err = dsService.UpdateDataSource(context.Background(), cmd) + require.EqualError(t, err, "[datasource.urlInvalid] max length is 255") + }) + + t.Run("should return no error if updated datasource", func(t *testing.T) { + sqlStore := db.InitTestDB(t) + secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore()) + secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger")) + quotaService := quotatest.New(false, nil) + mockPermission := acmock.NewMockedPermissionsService() + dsService, err := ProvideService(sqlStore, secretsService, secretsStore, cfg, featuremgmt.WithFeatures(), actest.FakeAccessControl{}, mockPermission, quotaService) + require.NoError(t, err) + + mockPermission.On("SetPermissions", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]accesscontrol.ResourcePermission{}, nil) + + ds, err := dsService.AddDataSource(context.Background(), &datasources.AddDataSourceCommand{ + OrgID: 1, + Name: "test-datasource", + }) + require.NoError(t, err) + + cmd := &datasources.UpdateDataSourceCommand{ + ID: ds.ID, + OrgID: ds.OrgID, + Name: "test-datasource-updated", + } + + _, err = dsService.UpdateDataSource(context.Background(), cmd) + require.NoError(t, err) + }) + + t.Run("should return error if datasource with same name exist", func(t *testing.T) { + sqlStore := db.InitTestDB(t) + secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore()) + secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger")) + quotaService := quotatest.New(false, nil) + mockPermission := acmock.NewMockedPermissionsService() + dsService, err := ProvideService(sqlStore, secretsService, secretsStore, cfg, featuremgmt.WithFeatures(), actest.FakeAccessControl{}, mockPermission, quotaService) + require.NoError(t, err) + + mockPermission.On("SetPermissions", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]accesscontrol.ResourcePermission{}, nil) + + dsToUpdate, err := dsService.AddDataSource(context.Background(), &datasources.AddDataSourceCommand{ + OrgID: 1, + Name: "test-datasource", + }) + require.NoError(t, err) + + existingDs, err := dsService.AddDataSource(context.Background(), &datasources.AddDataSourceCommand{ + OrgID: 1, + Name: "name already taken", + }) + require.NoError(t, err) + + cmd := &datasources.UpdateDataSourceCommand{ + ID: dsToUpdate.ID, + OrgID: dsToUpdate.OrgID, + Name: existingDs.Name, + } + + _, err = dsService.UpdateDataSource(context.Background(), cmd) + require.ErrorIs(t, err, datasources.ErrDataSourceNameExists) + }) +} + func TestService_NameScopeResolver(t *testing.T) { retriever := &dataSourceMockRetriever{[]*datasources.DataSource{ {Name: "test-datasource", UID: "1"},