RBAC: Make access control metadata for folders work with nested folders (#66464)

* remove metadata for single folder listing * extendTests * remove ac metadata from dash and folder search results * remove test * remove one more test * put ac metadata back for single folder API responses * extend tests * remove ac metadata from folder frontend object * undo unneeded change * PR feedback Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com> --------- Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
2023-04-21 15:05:11 +01:00
parent 94294a72c8
commit 5d7433d820
10 changed files with 87 additions and 233 deletions
@@ -27,9 +27,8 @@ type Folder struct {
 }

 type FolderSearchHit struct {
-	Id            int64                  `json:"id"`
-	Uid           string                 `json:"uid"`
-	Title         string                 `json:"title"`
-	AccessControl accesscontrol.Metadata `json:"accessControl,omitempty"`
-	ParentUID     string                 `json:"parentUid,omitempty"`
+	Id        int64  `json:"id"`
+	Uid       string `json:"uid"`
+	Title     string `json:"title"`
+	ParentUID string `json:"parentUid,omitempty"`
 }
@@ -54,10 +54,8 @@ func (hs *HTTPServer) GetFolders(c *contextmodel.ReqContext) response.Response {
 		return apierrors.ToFolderErrorResponse(err)
 	}

-	uids := make(map[string]bool, len(folders))
 	result := make([]dtos.FolderSearchHit, 0)
 	for _, f := range folders {
-		uids[f.UID] = true
 		result = append(result, dtos.FolderSearchHit{
 			Id:        f.ID,
 			Uid:       f.UID,
@@ -66,13 +64,6 @@ func (hs *HTTPServer) GetFolders(c *contextmodel.ReqContext) response.Response {
 		})
 	}

-	metadata := hs.getMultiAccessControlMetadata(c, c.OrgID, dashboards.ScopeFoldersPrefix, uids)
-	if len(metadata) > 0 {
-		for i := range result {
-			result[i].AccessControl = metadata[result[i].Uid]
-		}
-	}
-
 	return response.JSON(http.StatusOK, result)
 }

@@ -318,6 +309,8 @@ func (hs *HTTPServer) newToFolderDto(c *contextmodel.ReqContext, g guardian.Dash
 		updater = hs.getUserLogin(c.Req.Context(), folder.UpdatedBy)
 	}

+	acMetadata, _ := hs.getFolderACMetadata(c, folder)
+
 	return dtos.Folder{
 		Id:            folder.ID,
 		Uid:           folder.UID,
@@ -333,11 +326,38 @@ func (hs *HTTPServer) newToFolderDto(c *contextmodel.ReqContext, g guardian.Dash
 		UpdatedBy:     updater,
 		Updated:       folder.Updated,
 		Version:       folder.Version,
-		AccessControl: hs.getAccessControlMetadata(c, c.OrgID, dashboards.ScopeFoldersPrefix, folder.UID),
+		AccessControl: acMetadata,
 		ParentUID:     folder.ParentUID,
 	}
 }

+func (hs *HTTPServer) getFolderACMetadata(c *contextmodel.ReqContext, f *folder.Folder) (accesscontrol.Metadata, error) {
+	if hs.AccessControl.IsDisabled() || !c.QueryBool("accesscontrol") {
+		return nil, nil
+	}
+
+	parents, err := hs.folderService.GetParents(c.Req.Context(), folder.GetParentsQuery{UID: f.UID, OrgID: c.OrgID})
+	if err != nil {
+		return nil, err
+	}
+
+	folderIDs := map[string]bool{f.UID: true}
+	for _, p := range parents {
+		folderIDs[p.UID] = true
+	}
+
+	allMetadata := hs.getMultiAccessControlMetadata(c, c.OrgID, dashboards.ScopeFoldersPrefix, folderIDs)
+	metadata := allMetadata[f.UID]
+
+	// Flatten metadata - if any parent has a permission, the child folder inherits it
+	for _, md := range allMetadata {
+		for action := range md {
+			metadata[action] = true
+		}
+	}
+	return metadata, nil
+}
+
 func (hs *HTTPServer) searchFolders(c *contextmodel.ReqContext) ([]*folder.Folder, error) {
 	searchQuery := search.Query{
 		SignedInUser: c.SignedInUser,
@@ -271,6 +271,7 @@ func testDescription(description string, expectedErr error) string {
 func TestHTTPServer_FolderMetadata(t *testing.T) {
 	setUpRBACGuardian(t)
 	folderService := &foldertest.FakeService{}
+	features := featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders)
 	server := SetupAPITestServer(t, func(hs *HTTPServer) {
 		hs.Cfg = &setting.Cfg{
 			RBACEnabled: true,
@@ -280,35 +281,7 @@ func TestHTTPServer_FolderMetadata(t *testing.T) {
 		hs.SearchService = &mockSearchService{
 			ExpectedResult: model.HitList{},
 		}
-	})
-
-	t.Run("Should attach access control metadata to multiple folders", func(t *testing.T) {
-		folderService.ExpectedFolders = []*folder.Folder{{UID: "1"}, {UID: "2"}, {UID: "3"}}
-
-		req := server.NewGetRequest("/api/folders?accesscontrol=true")
-		webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{
-			1: accesscontrol.GroupScopesByAction([]accesscontrol.Permission{
-				{Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersAll},
-				{Action: dashboards.ActionFoldersWrite, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("2")},
-			}),
-		}})
-
-		res, err := server.Send(req)
-		require.NoError(t, err)
-		defer func() { require.NoError(t, res.Body.Close()) }()
-		assert.Equal(t, http.StatusOK, res.StatusCode)
-
-		body := []dtos.FolderSearchHit{}
-		require.NoError(t, json.NewDecoder(res.Body).Decode(&body))
-
-		for _, f := range body {
-			assert.True(t, f.AccessControl[dashboards.ActionFoldersRead])
-			if f.Uid == "2" {
-				assert.True(t, f.AccessControl[dashboards.ActionFoldersWrite])
-			} else {
-				assert.False(t, f.AccessControl[dashboards.ActionFoldersWrite])
-			}
-		}
+		hs.Features = features
 	})

 	t.Run("Should attach access control metadata to folder response", func(t *testing.T) {
@@ -334,7 +307,38 @@ func TestHTTPServer_FolderMetadata(t *testing.T) {
 		assert.True(t, body.AccessControl[dashboards.ActionFoldersWrite])
 	})

-	t.Run("Should attach access control metadata to folder response", func(t *testing.T) {
+	t.Run("Should attach access control metadata to folder response with permissions cascading from nested folders", func(t *testing.T) {
+		folderService.ExpectedFolder = &folder.Folder{UID: "folderUid"}
+		folderService.ExpectedFolders = []*folder.Folder{{UID: "parentUid"}}
+		features = featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders)
+		defer func() {
+			features = featuremgmt.WithFeatures()
+			folderService.ExpectedFolders = nil
+		}()
+
+		req := server.NewGetRequest("/api/folders/folderUid?accesscontrol=true")
+		webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{
+			1: accesscontrol.GroupScopesByAction([]accesscontrol.Permission{
+				{Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersAll},
+				{Action: dashboards.ActionFoldersWrite, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("parentUid")},
+				{Action: dashboards.ActionDashboardsCreate, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("folderUid")},
+			}),
+		}})
+
+		res, err := server.Send(req)
+		require.NoError(t, err)
+		assert.Equal(t, http.StatusOK, res.StatusCode)
+		defer func() { require.NoError(t, res.Body.Close()) }()
+
+		body := dtos.Folder{}
+		require.NoError(t, json.NewDecoder(res.Body).Decode(&body))
+
+		assert.True(t, body.AccessControl[dashboards.ActionFoldersRead])
+		assert.True(t, body.AccessControl[dashboards.ActionFoldersWrite])
+		assert.True(t, body.AccessControl[dashboards.ActionDashboardsCreate])
+	})
+
+	t.Run("Should not attach access control metadata to folder response", func(t *testing.T) {
 		folderService.ExpectedFolder = &folder.Folder{UID: "folderUid"}

 		req := server.NewGetRequest("/api/folders/folderUid")
@@ -6,7 +6,6 @@ import (

 	"github.com/grafana/grafana/pkg/api/response"
 	"github.com/grafana/grafana/pkg/infra/metrics"
-	"github.com/grafana/grafana/pkg/services/accesscontrol"
 	contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model"
 	"github.com/grafana/grafana/pkg/services/dashboards"
 	"github.com/grafana/grafana/pkg/services/search"
@@ -88,46 +87,7 @@ func (hs *HTTPServer) Search(c *contextmodel.ReqContext) response.Response {

 	defer c.TimeRequest(metrics.MApiDashboardSearch)

-	if !c.QueryBool("accesscontrol") {
-		return response.JSON(http.StatusOK, hits)
-	}
-
-	return hs.searchHitsWithMetadata(c, hits)
-}
-
-func (hs *HTTPServer) searchHitsWithMetadata(c *contextmodel.ReqContext, hits model.HitList) response.Response {
-	folderUIDs := make(map[string]bool)
-	dashboardUIDs := make(map[string]bool)
-
-	for _, hit := range hits {
-		if hit.Type == model.DashHitFolder {
-			folderUIDs[hit.UID] = true
-		} else {
-			dashboardUIDs[hit.UID] = true
-			folderUIDs[hit.FolderUID] = true
-		}
-	}
-
-	folderMeta := hs.getMultiAccessControlMetadata(c, c.OrgID, dashboards.ScopeFoldersPrefix, folderUIDs)
-	dashboardMeta := hs.getMultiAccessControlMetadata(c, c.OrgID, dashboards.ScopeDashboardsPrefix, dashboardUIDs)
-
-	// search hit with access control metadata attached
-	type hitWithMeta struct {
-		*model.Hit
-		AccessControl accesscontrol.Metadata `json:"accessControl,omitempty"`
-	}
-	hitsWithMeta := make([]hitWithMeta, 0, len(hits))
-	for _, hit := range hits {
-		var meta accesscontrol.Metadata
-		if hit.Type == model.DashHitFolder {
-			meta = folderMeta[hit.UID]
-		} else {
-			meta = accesscontrol.MergeMeta("dashboards", dashboardMeta[hit.UID], folderMeta[hit.FolderUID])
-		}
-		hitsWithMeta = append(hitsWithMeta, hitWithMeta{hit, meta})
-	}
-
-	return response.JSON(http.StatusOK, hitsWithMeta)
+	return response.JSON(http.StatusOK, hits)
 }

 // swagger:route GET /search/sorting search listSortOptions
@@ -1,101 +0,0 @@
-package api
-
-import (
-	"encoding/json"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-
-	"github.com/grafana/grafana/pkg/services/accesscontrol"
-	"github.com/grafana/grafana/pkg/services/dashboards"
-	"github.com/grafana/grafana/pkg/services/search/model"
-	"github.com/grafana/grafana/pkg/setting"
-	"github.com/grafana/grafana/pkg/web/webtest"
-)
-
-func TestHTTPServer_Search(t *testing.T) {
-	type testCase struct {
-		desc             string
-		includeMetadata  bool
-		permissions      []accesscontrol.Permission
-		expectedMetadata map[int64]map[string]struct{}
-	}
-
-	type withMeta struct {
-		model.Hit
-		AccessControl accesscontrol.Metadata `json:"accessControl,omitempty"`
-	}
-
-	tests := []testCase{
-		{
-			desc:            "should attach metadata to response",
-			includeMetadata: true,
-			expectedMetadata: map[int64]map[string]struct{}{
-				1: {dashboards.ActionFoldersRead: {}},
-				2: {dashboards.ActionFoldersRead: {}, dashboards.ActionFoldersWrite: {}, dashboards.ActionDashboardsWrite: {}},
-				3: {dashboards.ActionDashboardsRead: {}, dashboards.ActionDashboardsWrite: {}},
-			},
-			permissions: []accesscontrol.Permission{
-				{Action: "folders:read", Scope: "folders:*"},
-				{Action: "folders:write", Scope: "folders:uid:folder2"},
-				{Action: "dashboards:read", Scope: "dashboards:*"},
-				{Action: "dashboards:write", Scope: "folders:uid:folder2"},
-			},
-		},
-		{
-			desc:             "not attach metadata",
-			includeMetadata:  false,
-			expectedMetadata: map[int64]map[string]struct{}{},
-			permissions: []accesscontrol.Permission{
-				{Action: "folders:read", Scope: "folders:*"},
-				{Action: "folders:write", Scope: "folders:uid:folder2"},
-				{Action: "dashboards:read", Scope: "dashboards:*"},
-				{Action: "dashboards:write", Scope: "folders:uid:folder2"},
-			},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.desc, func(t *testing.T) {
-			server := SetupAPITestServer(t, func(hs *HTTPServer) {
-				hs.Cfg = setting.NewCfg()
-				hs.SearchService = &mockSearchService{ExpectedResult: model.HitList{
-					{ID: 1, UID: "folder1", Title: "folder1", Type: model.DashHitFolder},
-					{ID: 2, UID: "folder2", Title: "folder2", Type: model.DashHitFolder},
-					{ID: 3, UID: "dash3", Title: "dash3", FolderUID: "folder2", Type: model.DashHitDB},
-				}}
-			})
-
-			url := "/api/search"
-			if tt.includeMetadata {
-				url += "?accesscontrol=true"
-			}
-
-			res, err := server.Send(
-				webtest.RequestWithSignedInUser(
-					server.NewGetRequest(url), userWithPermissions(1, tt.permissions),
-				),
-			)
-			require.NoError(t, err)
-
-			var result []withMeta
-			require.NoError(t, json.NewDecoder(res.Body).Decode(&result))
-
-			for _, r := range result {
-				if !tt.includeMetadata {
-					assert.Nil(t, r.AccessControl)
-					continue
-				}
-
-				assert.Len(t, r.AccessControl, len(tt.expectedMetadata[r.ID]))
-				for action := range r.AccessControl {
-					_, ok := tt.expectedMetadata[r.ID][action]
-					assert.True(t, ok)
-				}
-			}
-
-			require.NoError(t, res.Body.Close())
-		})
-	}
-}