From 5cf40cbd125e36f43c0542350af9de1f9c5acc4a Mon Sep 17 00:00:00 2001 From: Daniel Lee Date: Sat, 17 Jun 2017 02:33:53 +0200 Subject: [PATCH] WIP: move guardian logic for search into the sql query --- pkg/api/playlist.go | 2 +- pkg/api/playlist_play.go | 18 ++-- pkg/api/search.go | 6 +- pkg/services/guardian/guardian.go | 23 ----- pkg/services/search/handlers.go | 34 +------ pkg/services/search/handlers_test.go | 2 +- pkg/services/search/models.go | 9 +- pkg/services/sqlstore/dashboard.go | 26 ++++- pkg/services/sqlstore/dashboard_test.go | 123 +++++++++++++++++++++--- pkg/services/sqlstore/guardian.go | 2 +- pkg/services/sqlstore/guardian_test.go | 26 ----- 11 files changed, 153 insertions(+), 118 deletions(-) diff --git a/pkg/api/playlist.go b/pkg/api/playlist.go index a6c2da26dd8..040aef0474e 100644 --- a/pkg/api/playlist.go +++ b/pkg/api/playlist.go @@ -130,7 +130,7 @@ func GetPlaylistItems(c *middleware.Context) Response { func GetPlaylistDashboards(c *middleware.Context) Response { playlistId := c.ParamsInt64(":id") - playlists, err := LoadPlaylistDashboards(c.OrgId, c.UserId, playlistId) + playlists, err := LoadPlaylistDashboards(c.OrgId, c.SignedInUser, playlistId) if err != nil { return ApiError(500, "Could not load dashboards", err) } diff --git a/pkg/api/playlist_play.go b/pkg/api/playlist_play.go index 29a806ce23d..1d059e06be5 100644 --- a/pkg/api/playlist_play.go +++ b/pkg/api/playlist_play.go @@ -34,18 +34,18 @@ func populateDashboardsById(dashboardByIds []int64, dashboardIdOrder map[int64]i return result, nil } -func populateDashboardsByTag(orgId, userId int64, dashboardByTag []string, dashboardTagOrder map[string]int) dtos.PlaylistDashboardsSlice { +func populateDashboardsByTag(orgId int64, signedInUser *m.SignedInUser, dashboardByTag []string, dashboardTagOrder map[string]int) dtos.PlaylistDashboardsSlice { result := make(dtos.PlaylistDashboardsSlice, 0) if len(dashboardByTag) > 0 { for _, tag := range dashboardByTag { searchQuery := search.Query{ - Title: "", - Tags: []string{tag}, - UserId: userId, - Limit: 100, - IsStarred: false, - OrgId: orgId, + Title: "", + Tags: []string{tag}, + SignedInUser: signedInUser, + Limit: 100, + IsStarred: false, + OrgId: orgId, } if err := bus.Dispatch(&searchQuery); err == nil { @@ -64,7 +64,7 @@ func populateDashboardsByTag(orgId, userId int64, dashboardByTag []string, dashb return result } -func LoadPlaylistDashboards(orgId, userId, playlistId int64) (dtos.PlaylistDashboardsSlice, error) { +func LoadPlaylistDashboards(orgId int64, signedInUser *m.SignedInUser, playlistId int64) (dtos.PlaylistDashboardsSlice, error) { playlistItems, _ := LoadPlaylistItems(playlistId) dashboardByIds := make([]int64, 0) @@ -89,7 +89,7 @@ func LoadPlaylistDashboards(orgId, userId, playlistId int64) (dtos.PlaylistDashb var k, _ = populateDashboardsById(dashboardByIds, dashboardIdOrder) result = append(result, k...) - result = append(result, populateDashboardsByTag(orgId, userId, dashboardByTag, dashboardTagOrder)...) + result = append(result, populateDashboardsByTag(orgId, signedInUser, dashboardByTag, dashboardTagOrder)...) sort.Sort(result) return result, nil diff --git a/pkg/api/search.go b/pkg/api/search.go index bf6e5ad91fe..17b1ac0da61 100644 --- a/pkg/api/search.go +++ b/pkg/api/search.go @@ -26,9 +26,9 @@ func Search(c *middleware.Context) { mode = "list" } - dbids := make([]int, 0) + dbids := make([]int64, 0) for _, id := range c.QueryStrings("dashboardIds") { - dashboardId, err := strconv.Atoi(id) + dashboardId, err := strconv.ParseInt(id, 10, 64) if err == nil { dbids = append(dbids, dashboardId) } @@ -37,7 +37,7 @@ func Search(c *middleware.Context) { searchQuery := search.Query{ Title: query, Tags: tags, - UserId: c.UserId, + SignedInUser: c.SignedInUser, Limit: limit, IsStarred: starred == "true", OrgId: c.OrgId, diff --git a/pkg/services/guardian/guardian.go b/pkg/services/guardian/guardian.go index 3ed65c85465..6340efee05f 100644 --- a/pkg/services/guardian/guardian.go +++ b/pkg/services/guardian/guardian.go @@ -5,22 +5,6 @@ import ( m "github.com/grafana/grafana/pkg/models" ) -// FilterRestrictedDashboards filters out dashboards from the list that the user does have access to -func FilterRestrictedDashboards(dashList []int64, orgId int64, userId int64) ([]int64, error) { - user, err := getUser(userId) - if err != nil { - return nil, err - } - - if user.IsGrafanaAdmin || user.OrgRole == m.ROLE_ADMIN { - return dashList, nil - } - - filteredList, err := getAllowedDashboards(dashList, orgId, userId) - - return filteredList, err -} - // CanViewAcl determines if a user has permission to view a dashboard's ACL func CanViewAcl(dashboardId int64, role m.RoleType, isGrafanaAdmin bool, orgId int64, userId int64) (bool, error) { if role == m.ROLE_ADMIN || isGrafanaAdmin { @@ -115,13 +99,6 @@ func checkPermission(minimumPermission m.PermissionType, permissions []*m.Dashbo return false, nil } -func getUser(userId int64) (*m.SignedInUser, error) { - query := m.GetSignedInUserQuery{UserId: userId} - err := bus.Dispatch(&query) - - return query.Result, err -} - func getAllowedDashboards(dashList []int64, orgId int64, userId int64) ([]int64, error) { query := m.GetAllowedDashboardsQuery{UserId: userId, OrgId: orgId, DashList: dashList} err := bus.Dispatch(&query) diff --git a/pkg/services/search/handlers.go b/pkg/services/search/handlers.go index e2ad56e7363..1b7eab79720 100644 --- a/pkg/services/search/handlers.go +++ b/pkg/services/search/handlers.go @@ -7,7 +7,6 @@ import ( "github.com/grafana/grafana/pkg/bus" m "github.com/grafana/grafana/pkg/models" - "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/setting" ) @@ -41,9 +40,8 @@ func searchHandler(query *Query) error { dashQuery := FindPersistedDashboardsQuery{ Title: query.Title, - UserId: query.UserId, + SignedInUser: query.SignedInUser, IsStarred: query.IsStarred, - OrgId: query.OrgId, DashboardIds: query.DashboardIds, Type: query.Type, ParentId: query.FolderId, @@ -76,11 +74,6 @@ func searchHandler(query *Query) error { hits = filtered } - hits, err := removeRestrictedDashboardsFromList(hits, query) - if err != nil { - return err - } - // sort main result array sort.Sort(hits) @@ -94,7 +87,7 @@ func searchHandler(query *Query) error { } // add isStarred info - if err := setIsStarredFlagOnSearchResults(query.UserId, hits); err != nil { + if err := setIsStarredFlagOnSearchResults(query.SignedInUser.UserId, hits); err != nil { return err } @@ -102,29 +95,6 @@ func searchHandler(query *Query) error { return nil } -func removeRestrictedDashboardsFromList(hits HitList, query *Query) (HitList, error) { - var dashboardIds = []int64{} - for _, hit := range hits { - dashboardIds = append(dashboardIds, hit.Id) - } - - filteredHits, err := guardian.FilterRestrictedDashboards(dashboardIds, query.OrgId, query.UserId) - if err != nil { - return nil, err - } - - filtered := HitList{} - for _, hit := range hits { - for _, dashId := range filteredHits { - if hit.Id == dashId { - filtered = append(filtered, hit) - } - } - } - - return filtered, nil -} - func stringInSlice(a string, list []string) bool { for _, b := range list { if b == a { diff --git a/pkg/services/search/handlers_test.go b/pkg/services/search/handlers_test.go index 3fd7801a323..18b7453271e 100644 --- a/pkg/services/search/handlers_test.go +++ b/pkg/services/search/handlers_test.go @@ -12,7 +12,7 @@ func TestSearch(t *testing.T) { Convey("Given search query", t, func() { jsonDashIndex = NewJsonDashIndex("../../../public/dashboards/") - query := Query{Limit: 2000} + query := Query{Limit: 2000, SignedInUser: &m.SignedInUser{IsGrafanaAdmin: true}} bus.AddHandler("test", func(query *FindPersistedDashboardsQuery) error { query.Result = HitList{ diff --git a/pkg/services/search/models.go b/pkg/services/search/models.go index d0468ac4576..8f6a42ac40a 100644 --- a/pkg/services/search/models.go +++ b/pkg/services/search/models.go @@ -1,6 +1,7 @@ package search import "strings" +import "github.com/grafana/grafana/pkg/models" type HitType string @@ -43,11 +44,11 @@ type Query struct { Title string Tags []string OrgId int64 - UserId int64 + SignedInUser *models.SignedInUser Limit int IsStarred bool Type string - DashboardIds []int + DashboardIds []int64 FolderId int64 Mode string @@ -57,9 +58,9 @@ type Query struct { type FindPersistedDashboardsQuery struct { Title string OrgId int64 - UserId int64 + SignedInUser *models.SignedInUser IsStarred bool - DashboardIds []int + DashboardIds []int64 Type string ParentId int64 Mode string diff --git a/pkg/services/sqlstore/dashboard.go b/pkg/services/sqlstore/dashboard.go index 489cc7022e6..169edbf6214 100644 --- a/pkg/services/sqlstore/dashboard.go +++ b/pkg/services/sqlstore/dashboard.go @@ -166,8 +166,10 @@ func findDashboards(query *search.FindPersistedDashboardsQuery) ([]DashboardSear dashboard.slug, dashboard_tag.term, dashboard.is_folder, - dashboard.parent_id + dashboard.parent_id, + pd.title as folder_title FROM dashboard + LEFT OUTER JOIN dashboard pd on pd.id = dashboard.parent_id LEFT OUTER JOIN dashboard_tag on dashboard_tag.dashboard_id = dashboard.id`) if query.IsStarred { @@ -175,12 +177,11 @@ func findDashboards(query *search.FindPersistedDashboardsQuery) ([]DashboardSear } sql.WriteString(` WHERE dashboard.org_id=?`) - - params = append(params, query.OrgId) + params = append(params, query.SignedInUser.OrgId) if query.IsStarred { sql.WriteString(` AND star.user_id=?`) - params = append(params, query.UserId) + params = append(params, query.SignedInUser.UserId) } if len(query.DashboardIds) > 0 { @@ -196,6 +197,23 @@ func findDashboards(query *search.FindPersistedDashboardsQuery) ([]DashboardSear sql.WriteString(")") } + if query.SignedInUser.OrgRole != m.ROLE_ADMIN { + allowedDashboardsSubQuery := ` AND (dashboard.has_acl = 0 OR dashboard.id in ( + SELECT distinct d.id AS DashboardId + FROM dashboard AS d + LEFT JOIN dashboard AS df ON d.parent_id = df.id + LEFT JOIN dashboard_acl as dfa on d.parent_id = dfa.dashboard_id or d.id = dfa.dashboard_id + LEFT JOIN user_group_member as ugm on ugm.user_group_id = dfa.user_group_id + WHERE + d.has_acl = 1 and + (dfa.user_id = ? or ugm.user_id = ?) + and d.org_id = ? + ))` + + sql.WriteString(allowedDashboardsSubQuery) + params = append(params, query.SignedInUser.UserId, query.SignedInUser.UserId, query.SignedInUser.OrgId) + } + if len(query.Title) > 0 { sql.WriteString(" AND dashboard.title " + dialect.LikeStr() + " ?") params = append(params, "%"+query.Title+"%") diff --git a/pkg/services/sqlstore/dashboard_test.go b/pkg/services/sqlstore/dashboard_test.go index 9a8d2312e5d..6ba4d5c0a13 100644 --- a/pkg/services/sqlstore/dashboard_test.go +++ b/pkg/services/sqlstore/dashboard_test.go @@ -9,6 +9,7 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/search" + "github.com/grafana/grafana/pkg/setting" ) func insertTestDashboard(title string, orgId int64, parentId int64, isFolder bool, tags ...interface{}) *m.Dashboard { @@ -116,9 +117,10 @@ func TestDashboardDataAccess(t *testing.T) { Convey("Should be able to search for dashboard and return in folder hierarchy", func() { query := search.FindPersistedDashboardsQuery{ - Title: "test dash 23", - OrgId: 1, - Mode: "tree", + Title: "test dash 23", + OrgId: 1, + Mode: "tree", + SignedInUser: &m.SignedInUser{OrgId: 1}, } err := SearchDashboards(&query) @@ -135,8 +137,9 @@ func TestDashboardDataAccess(t *testing.T) { Convey("Should be able to search for dashboard folder", func() { query := search.FindPersistedDashboardsQuery{ - Title: "1 test dash folder", - OrgId: 1, + Title: "1 test dash folder", + OrgId: 1, + SignedInUser: &m.SignedInUser{OrgId: 1}, } err := SearchDashboards(&query) @@ -149,8 +152,9 @@ func TestDashboardDataAccess(t *testing.T) { Convey("Should be able to search for a dashboard folder's children", func() { query := search.FindPersistedDashboardsQuery{ - OrgId: 1, - ParentId: savedFolder.Id, + OrgId: 1, + ParentId: savedFolder.Id, + SignedInUser: &m.SignedInUser{OrgId: 1}, } err := SearchDashboards(&query) @@ -164,9 +168,9 @@ func TestDashboardDataAccess(t *testing.T) { Convey("Should be able to search for dashboard by dashboard ids", func() { Convey("should be able to find two dashboards by id", func() { query := search.FindPersistedDashboardsQuery{ - DashboardIds: []int{2, 3}, - OrgId: 1, + DashboardIds: []int64{2, 3}, Mode: "tree", + SignedInUser: &m.SignedInUser{OrgId: 1}, } err := SearchDashboards(&query) @@ -183,8 +187,8 @@ func TestDashboardDataAccess(t *testing.T) { Convey("DashboardIds that does not exists should not cause errors", func() { query := search.FindPersistedDashboardsQuery{ - DashboardIds: []int{1000}, - OrgId: 1, + DashboardIds: []int64{1000}, + SignedInUser: &m.SignedInUser{OrgId: 1}, } err := SearchDashboards(&query) @@ -253,8 +257,9 @@ func TestDashboardDataAccess(t *testing.T) { So(err, ShouldBeNil) query := search.FindPersistedDashboardsQuery{ - OrgId: 1, - ParentId: savedFolder.Id, + OrgId: 1, + ParentId: savedFolder.Id, + SignedInUser: &m.SignedInUser{}, } err = SearchDashboards(&query) @@ -285,7 +290,7 @@ func TestDashboardDataAccess(t *testing.T) { }) Convey("Should be able to search for starred dashboards", func() { - query := search.FindPersistedDashboardsQuery{OrgId: 1, UserId: 10, IsStarred: true} + query := search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: 10, OrgId: 1}, IsStarred: true} err := SearchDashboards(&query) So(err, ShouldBeNil) @@ -294,5 +299,95 @@ func TestDashboardDataAccess(t *testing.T) { }) }) }) + + Convey("Given one dashboard folder with two dashboard and one dashboard in the root folder", func() { + folder := insertTestDashboard("1 test dash folder", 1, 0, true, "prod", "webapp") + dashInRoot := insertTestDashboard("test dash 67", 1, 0, false, "prod", "webapp") + insertTestDashboard("test dash 23", 1, folder.Id, false, "prod", "webapp") + insertTestDashboard("test dash 45", 1, folder.Id, false, "prod") + + currentUser := createUser("viewer", "Viewer", false) + + Convey("and no acls are set", func() { + Convey("should return all dashboards", func() { + query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, dashInRoot.Id}} + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 2) + So(query.Result[0].Id, ShouldEqual, folder.Id) + So(query.Result[1].Id, ShouldEqual, dashInRoot.Id) + }) + }) + + Convey("and acl is set for dashboard folder", func() { + var otherUser int64 = 999 + updateTestDashboardWithAcl(folder.Id, otherUser, m.PERMISSION_EDIT) + + Convey("should not return folder", func() { + query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, dashInRoot.Id}} + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 1) + So(query.Result[0].Id, ShouldEqual, dashInRoot.Id) + }) + + Convey("when the user is given permission", func() { + updateTestDashboardWithAcl(folder.Id, currentUser.Id, m.PERMISSION_EDIT) + + Convey("should be able to access folder", func() { + query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, dashInRoot.Id}} + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 2) + So(query.Result[0].Id, ShouldEqual, folder.Id) + So(query.Result[1].Id, ShouldEqual, dashInRoot.Id) + }) + }) + + Convey("when the user is an admin", func() { + Convey("should be able to access folder", func() { + query := &search.FindPersistedDashboardsQuery{ + SignedInUser: &m.SignedInUser{ + UserId: currentUser.Id, + OrgId: 1, + OrgRole: m.ROLE_ADMIN, + }, + OrgId: 1, + DashboardIds: []int64{folder.Id, dashInRoot.Id}, + } + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 2) + So(query.Result[0].Id, ShouldEqual, folder.Id) + So(query.Result[1].Id, ShouldEqual, dashInRoot.Id) + }) + }) + }) + }) }) } + +func createUser(name string, role string, isAdmin bool) m.User { + setting.AutoAssignOrg = true + setting.AutoAssignOrgRole = role + + currentUserCmd := m.CreateUserCommand{Login: name, Email: name + "@test.com", Name: "a " + name, IsAdmin: isAdmin} + err := CreateUser(¤tUserCmd) + So(err, ShouldBeNil) + + q1 := m.GetUserOrgListQuery{UserId: currentUserCmd.Result.Id} + GetUserOrgList(&q1) + So(q1.Result[0].Role, ShouldEqual, role) + + return currentUserCmd.Result +} + +func updateTestDashboardWithAcl(dashId int64, userId int64, permissionType m.PermissionType) { + err := AddOrUpdateDashboardPermission(&m.AddOrUpdateDashboardPermissionCommand{ + OrgId: 1, + UserId: userId, + DashboardId: dashId, + PermissionType: permissionType, + }) + So(err, ShouldBeNil) +} diff --git a/pkg/services/sqlstore/guardian.go b/pkg/services/sqlstore/guardian.go index f3e5d45e707..5e907434d40 100644 --- a/pkg/services/sqlstore/guardian.go +++ b/pkg/services/sqlstore/guardian.go @@ -21,7 +21,7 @@ from dashboard as d left join dashboard_acl as dfa on d.parent_id = dfa.dashboard_id or d.id = dfa.dashboard_id left join user_group_member as ugm on ugm.user_group_id = dfa.user_group_id where ( - (d.has_acl = 1 and (dfa.user_id = ? or ugm.user_id = ? or df.created_by = ? or (d.is_folder = 1 and d.created_by = ?))) + (d.has_acl = 1 and (dfa.user_id = ? or ugm.user_id = ?)) or d.has_acl = 0) and d.org_id = ?` diff --git a/pkg/services/sqlstore/guardian_test.go b/pkg/services/sqlstore/guardian_test.go index dfdc51d675f..4c4d47a3e06 100644 --- a/pkg/services/sqlstore/guardian_test.go +++ b/pkg/services/sqlstore/guardian_test.go @@ -4,7 +4,6 @@ import ( "testing" m "github.com/grafana/grafana/pkg/models" - "github.com/grafana/grafana/pkg/setting" . "github.com/smartystreets/goconvey/convey" ) @@ -60,28 +59,3 @@ func TestGuardianDataAccess(t *testing.T) { }) }) } - -func createUser(name string, role string, isAdmin bool) m.User { - setting.AutoAssignOrg = true - setting.AutoAssignOrgRole = role - - currentUserCmd := m.CreateUserCommand{Login: name, Email: name + "@test.com", Name: "a " + name, IsAdmin: isAdmin} - err := CreateUser(¤tUserCmd) - So(err, ShouldBeNil) - - q1 := m.GetUserOrgListQuery{UserId: currentUserCmd.Result.Id} - GetUserOrgList(&q1) - So(q1.Result[0].Role, ShouldEqual, role) - - return currentUserCmd.Result -} - -func updateTestDashboardWithAcl(dashId int64, userId int64, permissionType m.PermissionType) { - err := AddOrUpdateDashboardPermission(&m.AddOrUpdateDashboardPermissionCommand{ - OrgId: 1, - UserId: userId, - DashboardId: dashId, - PermissionType: permissionType, - }) - So(err, ShouldBeNil) -}