From b0448b92e5cc2678eb38e8021128d9d059d67884 Mon Sep 17 00:00:00 2001 From: Ieva Date: Thu, 16 Nov 2023 11:11:35 +0000 Subject: [PATCH] Dashboards: Allow updating a dashboard if the user doesn't have access to the parent folder (#78075) * change where folder checks are done for dash creation/updates * add test for folder not being found * test fixes * more test fixes * add nlint directive to where folder IDs are used * fix bad merge * fix test --- pkg/api/dashboard.go | 20 -------------- pkg/api/dashboard_test.go | 26 ------------------- pkg/services/dashboards/database/database.go | 17 +----------- .../dashboards/service/dashboard_service.go | 17 ++++++++++++ .../dashboard_service_integration_test.go | 2 +- .../service/dashboard_service_test.go | 10 +++++++ pkg/services/folder/folderimpl/folder.go | 11 ++++---- pkg/services/folder/folderimpl/folder_test.go | 9 ++++--- .../api/dashboards/api_dashboards_test.go | 2 +- 9 files changed, 42 insertions(+), 72 deletions(-) diff --git a/pkg/api/dashboard.go b/pkg/api/dashboard.go index 1d76407e7cb..3694037f6c3 100644 --- a/pkg/api/dashboard.go +++ b/pkg/api/dashboard.go @@ -25,7 +25,6 @@ import ( "github.com/grafana/grafana/pkg/services/dashboards" dashver "github.com/grafana/grafana/pkg/services/dashboardversion" "github.com/grafana/grafana/pkg/services/featuremgmt" - "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/org" pref "github.com/grafana/grafana/pkg/services/preference" @@ -388,25 +387,6 @@ func (hs *HTTPServer) postDashboard(c *contextmodel.ReqContext, cmd dashboards.S cmd.OrgID = c.SignedInUser.GetOrgID() cmd.UserID = userID - // nolint:staticcheck - if cmd.FolderUID != "" || cmd.FolderID != 0 { - folder, err := hs.folderService.Get(ctx, &folder.GetFolderQuery{ - OrgID: c.SignedInUser.GetOrgID(), - UID: &cmd.FolderUID, - // nolint:staticcheck - ID: &cmd.FolderID, - SignedInUser: c.SignedInUser, - }) - if err != nil { - if errors.Is(err, dashboards.ErrFolderNotFound) { - return response.Error(http.StatusBadRequest, "Folder not found", err) - } - return response.Error(http.StatusInternalServerError, "Error while checking folder ID", err) - } - // nolint:staticcheck - cmd.FolderID = folder.ID - cmd.FolderUID = folder.UID - } dash := cmd.GetDashboardModel() newDashboard := dash.ID == 0 diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index 398415119b2..a195b451603 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -3,7 +3,6 @@ package api import ( "context" "encoding/json" - "errors" "fmt" "net/http" "os" @@ -468,31 +467,6 @@ func TestDashboardAPIEndpoint(t *testing.T) { }) }) - t.Run("Given a request with incorrect folder uid for creating a dashboard with", func(t *testing.T) { - cmd := dashboards.SaveDashboardCommand{ - OrgID: 1, - UserID: 5, - Dashboard: simplejson.NewFromAny(map[string]any{ - "title": "Dash", - }), - Overwrite: true, - FolderUID: "folderUID", - IsFolder: false, - Message: "msg", - } - - dashboardService := dashboards.NewFakeDashboardService(t) - - mockFolder := &foldertest.FakeService{ - ExpectedError: errors.New("Error while searching Folder ID"), - } - - postDashboardScenario(t, "When calling POST on", "/api/dashboards", "/api/dashboards", cmd, dashboardService, mockFolder, func(sc *scenarioContext) { - callPostDashboard(sc) - assert.Equal(t, http.StatusInternalServerError, sc.resp.Code) - }) - }) - // This tests that invalid requests returns expected error responses t.Run("Given incorrect requests for creating a dashboard", func(t *testing.T) { testCases := []struct { diff --git a/pkg/services/dashboards/database/database.go b/pkg/services/dashboards/database/database.go index 66d70e1ddc8..a49a0b4391f 100644 --- a/pkg/services/dashboards/database/database.go +++ b/pkg/services/dashboards/database/database.go @@ -300,20 +300,6 @@ func getExistingDashboardByIDOrUIDForUpdate(sess *db.Session, dash *dashboards.D } } - // nolint:staticcheck - if dash.FolderID > 0 { - var existingFolder dashboards.Dashboard - folderExists, err := sess.Where("org_id=? AND id=? AND is_folder=?", dash.OrgID, dash.FolderID, - dialect.BooleanStr(true)).Get(&existingFolder) - if err != nil { - return false, fmt.Errorf("SQL query for folder failed: %w", err) - } - - if !folderExists { - return false, dashboards.ErrDashboardFolderNotFound - } - } - if !dashWithIdExists && !dashWithUidExists { return false, nil } @@ -335,8 +321,7 @@ func getExistingDashboardByIDOrUIDForUpdate(sess *db.Session, dash *dashboards.D return isParentFolderChanged, dashboards.ErrDashboardTypeMismatch } - // nolint:staticcheck - if !dash.IsFolder && dash.FolderID != existing.FolderID { + if !dash.IsFolder && dash.FolderUID != existing.FolderUID { isParentFolderChanged = true } diff --git a/pkg/services/dashboards/service/dashboard_service.go b/pkg/services/dashboards/service/dashboard_service.go index 2e6d65579c2..8dfa7c308eb 100644 --- a/pkg/services/dashboards/service/dashboard_service.go +++ b/pkg/services/dashboards/service/dashboard_service.go @@ -132,6 +132,23 @@ func (dr *DashboardServiceImpl) BuildSaveDashboardCommand(ctx context.Context, d } } + // Validate folder + if dash.FolderUID != "" { + folder, err := dr.folderStore.GetFolderByUID(ctx, dash.OrgID, dash.FolderUID) + if err != nil { + return nil, err + } + // nolint:staticcheck + dash.FolderID = folder.ID + } else if dash.FolderID != 0 { // nolint:staticcheck + // nolint:staticcheck + folder, err := dr.folderStore.GetFolderByID(ctx, dash.OrgID, dash.FolderID) + if err != nil { + return nil, err + } + dash.FolderUID = folder.UID + } + isParentFolderChanged, err := dr.dashboardStore.ValidateDashboardBeforeSave(ctx, dash, dto.Overwrite) if err != nil { return nil, err diff --git a/pkg/services/dashboards/service/dashboard_service_integration_test.go b/pkg/services/dashboards/service/dashboard_service_integration_test.go index bbe8effe7e1..d9cf6d931f3 100644 --- a/pkg/services/dashboards/service/dashboard_service_integration_test.go +++ b/pkg/services/dashboards/service/dashboard_service_integration_test.go @@ -481,7 +481,7 @@ func TestIntegrationIntegratedDashboardService(t *testing.T) { } err := callSaveWithError(t, cmd, sc.sqlStore) - assert.Equal(t, dashboards.ErrDashboardFolderNotFound, err) + assert.Equal(t, dashboards.ErrFolderNotFound, err) }) permissionScenario(t, "When updating an existing dashboard by id without current version", canSave, diff --git a/pkg/services/dashboards/service/dashboard_service_test.go b/pkg/services/dashboards/service/dashboard_service_test.go index 3b8c7eb0ea7..24b013f0554 100644 --- a/pkg/services/dashboards/service/dashboard_service_test.go +++ b/pkg/services/dashboards/service/dashboard_service_test.go @@ -93,6 +93,16 @@ func TestDashboardService(t *testing.T) { } }) + t.Run("Should return validation error if a folder that is specified can't be found", func(t *testing.T) { + dto.Dashboard = dashboards.NewDashboard("Dash") + dto.Dashboard.FolderUID = "non-existing-folder" + folderStore := foldertest.FakeFolderStore{} + folderStore.On("GetFolderByUID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(nil, dashboards.ErrFolderNotFound).Once() + service.folderStore = &folderStore + _, err := service.SaveDashboard(context.Background(), dto, false) + require.Equal(t, err, dashboards.ErrFolderNotFound) + }) + t.Run("Should return validation error if dashboard is provisioned", func(t *testing.T) { fakeStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.Anything, mock.AnythingOfType("bool")).Return(true, nil).Once() fakeStore.On("GetProvisionedDataByDashboardID", mock.Anything, mock.AnythingOfType("int64")).Return(&dashboards.DashboardProvisioning{}, nil).Once() diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index 6172cfe4409..a9311dd727c 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -858,13 +858,14 @@ func (s *Service) buildSaveDashboardCommand(ctx context.Context, dto *dashboards return nil, dashboards.ErrDashboardTitleEmpty } - // nolint:staticcheck - if dash.IsFolder && dash.FolderID > 0 { - return nil, dashboards.ErrDashboardFolderCannotHaveParent + if strings.EqualFold(dash.Title, dashboards.RootFolderName) { + return nil, dashboards.ErrDashboardFolderNameExists } - if dash.IsFolder && strings.EqualFold(dash.Title, dashboards.RootFolderName) { - return nil, dashboards.ErrDashboardFolderNameExists + if dash.FolderUID != "" { + if _, err := s.dashboardFolderStore.GetFolderByUID(ctx, dash.OrgID, dash.FolderUID); err != nil { + return nil, err + } } if !util.IsValidShortUID(dash.UID) { diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index b0e752a63f5..bf75055b5d0 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -833,6 +833,7 @@ func TestNestedFolderService(t *testing.T) { dashboardFolderStore := foldertest.NewFakeFolderStore(t) dashboardFolderStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil) + dashboardFolderStore.On("GetFolderByUID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(&folder.Folder{}, nil) nestedFolderUser := &user.SignedInUser{UserID: 1, OrgID: orgID, Permissions: map[int64]map[string][]string{}} nestedFolderUser.Permissions[orgID] = map[string][]string{dashboards.ActionFoldersWrite: {dashboards.ScopeFoldersProvider.GetResourceScopeUID("some_parent")}} @@ -904,6 +905,7 @@ func TestNestedFolderService(t *testing.T) { dashboardFolderStore := foldertest.NewFakeFolderStore(t) dashboardFolderStore.On("GetFolderByID", mock.Anything, orgID, dashboardFolder.ID).Return(f, nil) + dashboardFolderStore.On("GetFolderByUID", mock.Anything, orgID, dashboardFolder.UID).Return(f, nil) nestedFolderStore := NewFakeStore() nestedFolderStore.ExpectedParentFolders = []*folder.Folder{ @@ -914,10 +916,10 @@ func TestNestedFolderService(t *testing.T) { } cmd := folder.CreateFolderCommand{ - ParentUID: "myFolder1", + ParentUID: dashboardFolder.UID, OrgID: orgID, - Title: "myFolder", - UID: "myFolder", + Title: "myFolder1", + UID: "myFolder1", SignedInUser: usr, } @@ -1135,6 +1137,7 @@ func TestNestedFolderService(t *testing.T) { dashboardFolderStore := foldertest.NewFakeFolderStore(t) dashboardFolderStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil) + dashboardFolderStore.On("GetFolderByUID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(&folder.Folder{}, nil) parents := make([]*folder.Folder, 0, folder.MaxNestedFolderDepth) for i := 0; i < folder.MaxNestedFolderDepth; i++ { diff --git a/pkg/tests/api/dashboards/api_dashboards_test.go b/pkg/tests/api/dashboards/api_dashboards_test.go index 2c4bf3f16b9..0e52f540ab9 100644 --- a/pkg/tests/api/dashboards/api_dashboards_test.go +++ b/pkg/tests/api/dashboards/api_dashboards_test.go @@ -405,7 +405,7 @@ func TestIntegrationCreate(t *testing.T) { var m util.DynMap err = json.Unmarshal(b, &m) require.NoError(t, err) - assert.Equal(t, "Folder not found", m["message"]) + assert.Equal(t, dashboards.ErrFolderNotFound.Error(), m["message"]) }) }