From 59a6a6513f512c8cb854443175c7aa612031aeaf Mon Sep 17 00:00:00 2001 From: Aaron Godin Date: Mon, 10 Jun 2024 09:17:51 -0500 Subject: [PATCH] Prevent moving a k6 folder (#88884) * iam-716 - prevent a folder move operation when the folder's uid or any of its parents uids begin with k6-app * fox folder move check and only list non-k6 folders to users * adding tests for moving * add a test for listing folders * fix the other tests * use method that adds folder parent --------- Co-authored-by: IevaVasiljeva --- pkg/api/folder.go | 6 ++ pkg/services/accesscontrol/models.go | 1 + .../dashboards/service/dashboard_service.go | 7 ++ pkg/services/folder/folderimpl/folder.go | 33 +++++--- pkg/services/folder/folderimpl/folder_test.go | 81 +++++++++++++++++++ 5 files changed, 119 insertions(+), 9 deletions(-) diff --git a/pkg/api/folder.go b/pkg/api/folder.go index 899f976a6a5..5b129f7c013 100644 --- a/pkg/api/folder.go +++ b/pkg/api/folder.go @@ -64,7 +64,13 @@ func (hs *HTTPServer) GetFolders(c *contextmodel.ReqContext) response.Response { } hits := make([]dtos.FolderSearchHit, 0) + requesterIsSvcAccount := c.SignedInUser.GetID().Namespace() == identity.NamespaceServiceAccount for _, f := range folders { + // only list k6 folders when requested by a service account - prevents showing k6 folders in the UI for users + if (f.UID == accesscontrol.K6FolderUID || f.ParentUID == accesscontrol.K6FolderUID) && !requesterIsSvcAccount { + continue + } + hits = append(hits, dtos.FolderSearchHit{ ID: f.ID, // nolint:staticcheck UID: f.UID, diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index d01f64dfeef..d59230e5443 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -332,6 +332,7 @@ const ( GlobalOrgID = 0 NoOrgID = int64(-1) GeneralFolderUID = "general" + K6FolderUID = "k6-app" RoleGrafanaAdmin = "Grafana Admin" // Permission actions diff --git a/pkg/services/dashboards/service/dashboard_service.go b/pkg/services/dashboards/service/dashboard_service.go index 677f7308ddd..57507af88db 100644 --- a/pkg/services/dashboards/service/dashboard_service.go +++ b/pkg/services/dashboards/service/dashboard_service.go @@ -700,9 +700,16 @@ func makeQueryResult(query *dashboards.FindPersistedDashboardsQuery, res []dashb hitList := make([]*model.Hit, 0) hits := make(map[int64]*model.Hit) + requesterIsSvcAccount := query.SignedInUser.GetID().Namespace() == identity.NamespaceServiceAccount + for _, item := range res { hit, exists := hits[item.ID] if !exists { + // Don't list k6 items for users, we don't want users to interact with k6 folders directly through folder UI + if (item.UID == accesscontrol.K6FolderUID || item.FolderUID == accesscontrol.K6FolderUID) && !requesterIsSvcAccount { + continue + } + metrics.MFolderIDsServiceCount.WithLabelValues(metrics.Dashboard).Inc() hit = &model.Hit{ ID: item.ID, diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index e70f220ea94..4411819fbb9 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -180,7 +180,17 @@ func (s *Service) GetFolders(ctx context.Context, q folder.GetFoldersQuery) ([]* } } - return dashFolders, nil + // only list k6 folders when requested by a service account - prevents showing k6 folders in the UI for users + result := make([]*folder.Folder, 0, len(dashFolders)) + requesterIsSvcAccount := qry.SignedInUser.GetID().Namespace() == identity.NamespaceServiceAccount + for _, folder := range dashFolders { + if (folder.UID == accesscontrol.K6FolderUID || folder.ParentUID == accesscontrol.K6FolderUID) && !requesterIsSvcAccount { + continue + } + result = append(result, folder) + } + + return result, nil } func (s *Service) Get(ctx context.Context, q *folder.GetFolderQuery) (*folder.Folder, error) { @@ -870,6 +880,16 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol return nil, folder.ErrBadRequest.Errorf("missing signed in user") } + // k6-specific check to prevent folder move for a k6-app folder and its children + if cmd.UID == accesscontrol.K6FolderUID { + return nil, folder.ErrBadRequest.Errorf("k6 project may not be moved") + } + if f, err := s.store.Get(ctx, folder.GetFolderQuery{UID: &cmd.UID, OrgID: cmd.OrgID}); err != nil { + return nil, err + } else if f != nil && f.ParentUID == accesscontrol.K6FolderUID { + return nil, folder.ErrBadRequest.Errorf("k6 project may not be moved") + } + // Check that the user is allowed to move the folder to the destination folder var evaluator accesscontrol.Evaluator if cmd.NewParentUID != "" { @@ -902,24 +922,19 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol return nil, folder.ErrMaximumDepthReached.Errorf("failed to move folder") } - // if the current folder is already a parent of newparent, we should return error for _, parent := range parents { + // if the current folder is already a parent of newparent, we should return error if parent.UID == cmd.UID { return nil, folder.ErrCircularReference.Errorf("failed to move folder") } } - newParentUID := "" - if cmd.NewParentUID != "" { - newParentUID = cmd.NewParentUID - } - var f *folder.Folder if err := s.db.InTransaction(ctx, func(ctx context.Context) error { if f, err = s.store.Update(ctx, folder.UpdateFolderCommand{ UID: cmd.UID, OrgID: cmd.OrgID, - NewParentUID: &newParentUID, + NewParentUID: &cmd.NewParentUID, SignedInUser: cmd.SignedInUser, }); err != nil { if s.db.GetDialect().IsUniqueConstraintViolation(err) { @@ -931,7 +946,7 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol if _, err := s.legacyUpdate(ctx, &folder.UpdateFolderCommand{ UID: cmd.UID, OrgID: cmd.OrgID, - NewParentUID: &newParentUID, + NewParentUID: &cmd.NewParentUID, SignedInUser: cmd.SignedInUser, // bypass optimistic locking used for dashboards Overwrite: true, diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index 633bc8605a4..5e9485e4bc8 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -1117,6 +1117,7 @@ func TestNestedFolderService(t *testing.T) { t.Run("move without the right permissions should fail", func(t *testing.T) { dashStore := &dashboards.FakeDashboardStore{} dashboardFolderStore := foldertest.NewFakeFolderStore(t) + //dashboardFolderStore.On("GetFolderByUID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(&folder.Folder{}, nil) nestedFolderStore := NewFakeStore() nestedFolderStore.ExpectedFolder = &folder.Folder{UID: "myFolder", ParentUID: "newFolder"} @@ -1153,6 +1154,34 @@ func TestNestedFolderService(t *testing.T) { // require.NotNil(t, f) }) + t.Run("cannot move the k6 folder even when has permissions to move folders", func(t *testing.T) { + nestedFolderUser := &user.SignedInUser{UserID: 1, OrgID: orgID, Permissions: map[int64]map[string][]string{}} + nestedFolderUser.Permissions[orgID] = map[string][]string{dashboards.ActionFoldersWrite: {dashboards.ScopeFoldersProvider.GetResourceAllScope()}} + + features := featuremgmt.WithFeatures("nestedFolders") + folderSvc := setup(t, &dashboards.FakeDashboardStore{}, foldertest.NewFakeFolderStore(t), NewFakeStore(), features, acimpl.ProvideAccessControl(features), dbtest.NewFakeDB()) + _, err := folderSvc.Move(context.Background(), &folder.MoveFolderCommand{UID: accesscontrol.K6FolderUID, NewParentUID: "newFolder", OrgID: orgID, SignedInUser: nestedFolderUser}) + require.Error(t, err, folder.ErrBadRequest) + }) + + t.Run("cannot move a k6 subfolder even when has permissions to move folders", func(t *testing.T) { + nestedFolderUser := &user.SignedInUser{UserID: 1, OrgID: orgID, Permissions: map[int64]map[string][]string{}} + nestedFolderUser.Permissions[orgID] = map[string][]string{dashboards.ActionFoldersWrite: {dashboards.ScopeFoldersProvider.GetResourceAllScope()}} + + childUID := "k6-app-child" + nestedFolderStore := NewFakeStore() + nestedFolderStore.ExpectedFolder = &folder.Folder{ + OrgID: orgID, + UID: childUID, + ParentUID: accesscontrol.K6FolderUID, + } + + features := featuremgmt.WithFeatures("nestedFolders") + folderSvc := setup(t, &dashboards.FakeDashboardStore{}, foldertest.NewFakeFolderStore(t), nestedFolderStore, features, acimpl.ProvideAccessControl(features), dbtest.NewFakeDB()) + _, err := folderSvc.Move(context.Background(), &folder.MoveFolderCommand{UID: childUID, NewParentUID: "newFolder", OrgID: orgID, SignedInUser: nestedFolderUser}) + require.Error(t, err, folder.ErrBadRequest) + }) + t.Run("move to the root folder without folder creation permissions fails", func(t *testing.T) { dashStore := &dashboards.FakeDashboardStore{} dashboardFolderStore := foldertest.NewFakeFolderStore(t) @@ -1461,6 +1490,58 @@ func TestIntegrationNestedFolderSharedWithMe(t *testing.T) { }) }) + t.Run("Should not list k6 folders or subfolders", func(t *testing.T) { + _, err = nestedFolderStore.Create(context.Background(), folder.CreateFolderCommand{ + UID: accesscontrol.K6FolderUID, + OrgID: orgID, + SignedInUser: &signedInAdminUser, + }) + require.NoError(t, err) + + k6ChildFolder, err := nestedFolderStore.Create(context.Background(), folder.CreateFolderCommand{ + UID: "k6-app-child", + ParentUID: accesscontrol.K6FolderUID, + OrgID: orgID, + SignedInUser: &signedInAdminUser, + }) + require.NoError(t, err) + + unrelatedFolder, err := nestedFolderStore.Create(context.Background(), folder.CreateFolderCommand{ + UID: "another-folder", + OrgID: orgID, + SignedInUser: &signedInAdminUser, + }) + require.NoError(t, err) + + folders, err := serviceWithFlagOn.GetFolders(context.Background(), folder.GetFoldersQuery{ + OrgID: orgID, + SignedInUser: &signedInAdminUser, + }) + require.NoError(t, err) + assert.Equal(t, 1, len(folders), "should not return k6 folders or subfolders") + assert.Equal(t, unrelatedFolder.UID, folders[0].UID) + + // Service accounts should be able to list k6 folders + svcAccountUser := user.SignedInUser{UserID: 2, IsServiceAccount: true, OrgID: orgID, Permissions: map[int64]map[string][]string{ + orgID: { + dashboards.ActionFoldersRead: {dashboards.ScopeFoldersAll}, + }, + }} + folders, err = serviceWithFlagOn.GetFolders(context.Background(), folder.GetFoldersQuery{ + OrgID: orgID, + SignedInUser: &svcAccountUser, + }) + require.NoError(t, err) + assert.Equal(t, 3, len(folders), "service accounts should be able to list k6 folders") + + t.Cleanup(func() { + //guardian.New = origNewGuardian + toDelete := []string{k6ChildFolder.UID, accesscontrol.K6FolderUID, unrelatedFolder.UID} + err := serviceWithFlagOn.store.Delete(context.Background(), toDelete, orgID) + assert.NoError(t, err) + }) + }) + t.Run("Should get org folders visible", func(t *testing.T) { depth := 3