From 9fcee9b2060814f61088826da400d94caf3b3cf5 Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> Date: Fri, 8 Sep 2023 10:43:41 +0300 Subject: [PATCH] Nested folders: Fix folder hierarchy in folder responses (#74516) Nested folders: Fix hierarchy in folder response --- pkg/api/folder.go | 60 ++++++++++++++++++++++++++++++------------ pkg/api/folder_test.go | 24 ++++++++++++++++- 2 files changed, 66 insertions(+), 18 deletions(-) diff --git a/pkg/api/folder.go b/pkg/api/folder.go index 68a44e60b8d..e682700770b 100644 --- a/pkg/api/folder.go +++ b/pkg/api/folder.go @@ -23,6 +23,8 @@ import ( "github.com/grafana/grafana/pkg/web" ) +const REDACTED = "redacted" + // swagger:route GET /folders folders getFolders // // Get all folders. @@ -87,12 +89,12 @@ func (hs *HTTPServer) GetFolderByUID(c *contextmodel.ReqContext) response.Respon return apierrors.ToFolderErrorResponse(err) } - g, err := guardian.NewByFolder(c.Req.Context(), folder, c.OrgID, c.SignedInUser) + folderDTO, err := hs.newToFolderDto(c, folder) if err != nil { return response.Err(err) } - return response.JSON(http.StatusOK, hs.newToFolderDto(c, g, folder)) + return response.JSON(http.StatusOK, folderDTO) } // swagger:route GET /folders/id/{folder_id} folders getFolderByID @@ -120,11 +122,11 @@ func (hs *HTTPServer) GetFolderByID(c *contextmodel.ReqContext) response.Respons return apierrors.ToFolderErrorResponse(err) } - g, err := guardian.NewByFolder(c.Req.Context(), folder, c.OrgID, c.SignedInUser) + folderDTO, err := hs.newToFolderDto(c, folder) if err != nil { return response.Err(err) } - return response.JSON(http.StatusOK, hs.newToFolderDto(c, g, folder)) + return response.JSON(http.StatusOK, folderDTO) } // swagger:route POST /folders folders createFolder @@ -161,13 +163,13 @@ func (hs *HTTPServer) CreateFolder(c *contextmodel.ReqContext) response.Response // Required for cases when caller wants to immediately interact with the newly created object hs.accesscontrolService.ClearUserPermissionCache(c.SignedInUser) - g, err := guardian.NewByFolder(c.Req.Context(), folder, c.OrgID, c.SignedInUser) + folderDTO, err := hs.newToFolderDto(c, folder) if err != nil { return response.Err(err) } // TODO set ParentUID if nested folders are enabled - return response.JSON(http.StatusOK, hs.newToFolderDto(c, g, folder)) + return response.JSON(http.StatusOK, folderDTO) } func (hs *HTTPServer) setDefaultFolderPermissions(ctx context.Context, orgID int64, user *user.SignedInUser, folder *folder.Folder) error { @@ -216,11 +218,11 @@ func (hs *HTTPServer) MoveFolder(c *contextmodel.ReqContext) response.Response { return response.Error(http.StatusInternalServerError, "move folder failed", err) } - g, err := guardian.NewByFolder(c.Req.Context(), theFolder, c.OrgID, c.SignedInUser) + folderDTO, err := hs.newToFolderDto(c, theFolder) if err != nil { return response.Err(err) } - return response.JSON(http.StatusOK, hs.newToFolderDto(c, g, theFolder)) + return response.JSON(http.StatusOK, folderDTO) } result := map[string]string{} result["message"] = "To use this service, you need to activate nested folder feature." @@ -252,12 +254,12 @@ func (hs *HTTPServer) UpdateFolder(c *contextmodel.ReqContext) response.Response if err != nil { return apierrors.ToFolderErrorResponse(err) } - g, err := guardian.NewByFolder(c.Req.Context(), result, c.OrgID, c.SignedInUser) + folderDTO, err := hs.newToFolderDto(c, result) if err != nil { return response.Err(err) } - return response.JSON(http.StatusOK, hs.newToFolderDto(c, g, result)) + return response.JSON(http.StatusOK, folderDTO) } // swagger:route DELETE /folders/{folder_uid} folders deleteFolder @@ -319,9 +321,14 @@ func (hs *HTTPServer) GetFolderDescendantCounts(c *contextmodel.ReqContext) resp return response.JSON(http.StatusOK, counts) } -func (hs *HTTPServer) newToFolderDto(c *contextmodel.ReqContext, g guardian.DashboardGuardian, f *folder.Folder) dtos.Folder { +func (hs *HTTPServer) newToFolderDto(c *contextmodel.ReqContext, f *folder.Folder) (dtos.Folder, error) { ctx := c.Req.Context() - toDTO := func(f *folder.Folder) dtos.Folder { + toDTO := func(f *folder.Folder, checkCanView bool) (dtos.Folder, error) { + g, err := guardian.NewByFolder(c.Req.Context(), f, c.OrgID, c.SignedInUser) + if err != nil { + return dtos.Folder{}, err + } + canEdit, _ := g.CanEdit() canSave, _ := g.CanSave() canAdmin, _ := g.CanAdmin() @@ -338,6 +345,16 @@ func (hs *HTTPServer) newToFolderDto(c *contextmodel.ReqContext, g guardian.Dash acMetadata, _ := hs.getFolderACMetadata(c, f) + if checkCanView { + canView, _ := g.CanView() + if !canView { + return dtos.Folder{ + Uid: REDACTED, + Title: REDACTED, + }, nil + } + } + return dtos.Folder{ Id: f.ID, Uid: f.UID, @@ -355,13 +372,17 @@ func (hs *HTTPServer) newToFolderDto(c *contextmodel.ReqContext, g guardian.Dash Version: f.Version, AccessControl: acMetadata, ParentUID: f.ParentUID, - } + }, nil } - folderDTO := toDTO(f) + // no need to check view permission for the starting folder since it's already checked by the callers + folderDTO, err := toDTO(f, false) + if err != nil { + return dtos.Folder{}, err + } if !hs.Features.IsEnabled(featuremgmt.FlagNestedFolders) { - return folderDTO + return folderDTO, nil } parents, err := hs.folderService.GetParents(ctx, folder.GetParentsQuery{UID: f.UID, OrgID: f.OrgID}) @@ -372,10 +393,15 @@ func (hs *HTTPServer) newToFolderDto(c *contextmodel.ReqContext, g guardian.Dash folderDTO.Parents = make([]dtos.Folder, 0, len(parents)) for _, f := range parents { - folderDTO.Parents = append(folderDTO.Parents, toDTO(f)) + DTO, err := toDTO(f, true) + if err != nil { + hs.log.Error("failed to convert folder to DTO", "folder", f.UID, "org", f.OrgID, "error", err) + continue + } + folderDTO.Parents = append(folderDTO.Parents, DTO) } - return folderDTO + return folderDTO, nil } func (hs *HTTPServer) getFolderACMetadata(c *contextmodel.ReqContext, f *folder.Folder) (accesscontrol.Metadata, error) { diff --git a/pkg/api/folder_test.go b/pkg/api/folder_test.go index 6ad16c110f0..b5c60bf8e3c 100644 --- a/pkg/api/folder_test.go +++ b/pkg/api/folder_test.go @@ -19,6 +19,7 @@ import ( "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/folder/foldertest" + "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/quota/quotatest" "github.com/grafana/grafana/pkg/services/search/model" "github.com/grafana/grafana/pkg/services/user" @@ -433,7 +434,6 @@ func TestFolderGetAPIEndpoint(t *testing.T) { }, }, } - setUpRBACGuardian(t) type testCase struct { description string @@ -443,6 +443,7 @@ func TestFolderGetAPIEndpoint(t *testing.T) { expectedParentUIDs []string expectedParentTitles []string permissions []accesscontrol.Permission + g *guardian.FakeDashboardGuardian } tcs := []testCase{ { @@ -455,6 +456,19 @@ func TestFolderGetAPIEndpoint(t *testing.T) { permissions: []accesscontrol.Permission{ {Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("uid")}, }, + g: &guardian.FakeDashboardGuardian{CanViewValue: true}, + }, + { + description: "get folder by UID should return parent folders redacted if nested folder are enabled and user does not have read access to parent folders", + URL: "/api/folders/uid", + expectedCode: http.StatusOK, + features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), + expectedParentUIDs: []string{REDACTED, REDACTED}, + expectedParentTitles: []string{REDACTED, REDACTED}, + permissions: []accesscontrol.Permission{ + {Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("uid")}, + }, + g: &guardian.FakeDashboardGuardian{CanViewValue: false}, }, { description: "get folder by UID should not return parent folders if nested folder are disabled", @@ -466,6 +480,7 @@ func TestFolderGetAPIEndpoint(t *testing.T) { permissions: []accesscontrol.Permission{ {Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("uid")}, }, + g: &guardian.FakeDashboardGuardian{CanViewValue: true}, }, } @@ -477,6 +492,13 @@ func TestFolderGetAPIEndpoint(t *testing.T) { }) t.Run(tc.description, func(t *testing.T) { + origNewGuardian := guardian.New + t.Cleanup(func() { + guardian.New = origNewGuardian + }) + + guardian.MockDashboardGuardian(tc.g) + req := srv.NewGetRequest(tc.URL) req = webtest.RequestWithSignedInUser(req, userWithPermissions(1, tc.permissions)) resp, err := srv.Send(req)