From fcaa8227a6d914ca2dbbcffedf7ce4c63d968264 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Wed, 14 Feb 2018 15:04:26 +0100 Subject: [PATCH] Dashboard acl query fixes (#10909) * initial fixes for dashboard permission acl list query, fixes #10864 * permissions: refactoring of acl api and query --- pkg/api/api.go | 1 - pkg/api/dashboard_acl.go | 30 ---- pkg/api/dashboard_acl_test.go | 104 ++----------- pkg/api/dashboard_test.go | 8 +- pkg/models/dashboard_acl.go | 16 -- pkg/services/guardian/guardian.go | 20 --- pkg/services/sqlstore/dashboard.go | 32 +--- pkg/services/sqlstore/dashboard_acl.go | 147 +++--------------- pkg/services/sqlstore/dashboard_acl_test.go | 91 ++++------- .../sqlstore/dashboard_folder_test.go | 29 ++-- pkg/services/sqlstore/dashboard_test.go | 19 --- pkg/services/sqlstore/org_test.go | 15 +- pkg/services/sqlstore/team_test.go | 2 +- pkg/services/sqlstore/user_test.go | 2 +- .../PermissionsStore/PermissionsStoreItem.ts | 3 +- 15 files changed, 89 insertions(+), 430 deletions(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index c03bf7963b8..1320663f630 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -269,7 +269,6 @@ func (hs *HttpServer) registerRoutes() { dashIdRoute.Group("/acl", func(aclRoute RouteRegister) { aclRoute.Get("/", wrap(GetDashboardAclList)) aclRoute.Post("/", bind(dtos.UpdateDashboardAclCommand{}), wrap(UpdateDashboardAcl)) - aclRoute.Delete("/:aclId", wrap(DeleteDashboardAcl)) }) }) }) diff --git a/pkg/api/dashboard_acl.go b/pkg/api/dashboard_acl.go index 45f121dd0d0..32b75e80cc0 100644 --- a/pkg/api/dashboard_acl.go +++ b/pkg/api/dashboard_acl.go @@ -84,33 +84,3 @@ func UpdateDashboardAcl(c *middleware.Context, apiCmd dtos.UpdateDashboardAclCom return ApiSuccess("Dashboard acl updated") } - -func DeleteDashboardAcl(c *middleware.Context) Response { - dashId := c.ParamsInt64(":dashboardId") - aclId := c.ParamsInt64(":aclId") - - _, rsp := getDashboardHelper(c.OrgId, "", dashId, "") - if rsp != nil { - return rsp - } - - guardian := guardian.NewDashboardGuardian(dashId, c.OrgId, c.SignedInUser) - if canAdmin, err := guardian.CanAdmin(); err != nil || !canAdmin { - return dashboardGuardianResponse(err) - } - - if okToDelete, err := guardian.CheckPermissionBeforeRemove(m.PERMISSION_ADMIN, aclId); err != nil || !okToDelete { - if err != nil { - return ApiError(500, "Error while checking dashboard permissions", err) - } - - return ApiError(403, "Cannot remove own admin permission for a folder", nil) - } - - cmd := m.RemoveDashboardAclCommand{OrgId: c.OrgId, AclId: aclId} - if err := bus.Dispatch(&cmd); err != nil { - return ApiError(500, "Failed to delete permission for user", err) - } - - return Json(200, "") -} diff --git a/pkg/api/dashboard_acl_test.go b/pkg/api/dashboard_acl_test.go index e43e57ed5c0..d6b7e305daf 100644 --- a/pkg/api/dashboard_acl_test.go +++ b/pkg/api/dashboard_acl_test.go @@ -15,11 +15,11 @@ import ( func TestDashboardAclApiEndpoint(t *testing.T) { Convey("Given a dashboard acl", t, func() { mockResult := []*m.DashboardAclInfoDTO{ - {Id: 1, OrgId: 1, DashboardId: 1, UserId: 2, Permission: m.PERMISSION_VIEW}, - {Id: 2, OrgId: 1, DashboardId: 1, UserId: 3, Permission: m.PERMISSION_EDIT}, - {Id: 3, OrgId: 1, DashboardId: 1, UserId: 4, Permission: m.PERMISSION_ADMIN}, - {Id: 4, OrgId: 1, DashboardId: 1, TeamId: 1, Permission: m.PERMISSION_VIEW}, - {Id: 5, OrgId: 1, DashboardId: 1, TeamId: 2, Permission: m.PERMISSION_ADMIN}, + {OrgId: 1, DashboardId: 1, UserId: 2, Permission: m.PERMISSION_VIEW}, + {OrgId: 1, DashboardId: 1, UserId: 3, Permission: m.PERMISSION_EDIT}, + {OrgId: 1, DashboardId: 1, UserId: 4, Permission: m.PERMISSION_ADMIN}, + {OrgId: 1, DashboardId: 1, TeamId: 1, Permission: m.PERMISSION_VIEW}, + {OrgId: 1, DashboardId: 1, TeamId: 2, Permission: m.PERMISSION_ADMIN}, } dtoRes := transformDashboardAclsToDTOs(mockResult) @@ -92,21 +92,11 @@ func TestDashboardAclApiEndpoint(t *testing.T) { So(sc.resp.Code, ShouldEqual, 404) }) }) - - loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/dashboards/id/2/acl/6", "/api/dashboards/id/:dashboardId/acl/:aclId", m.ROLE_ADMIN, func(sc *scenarioContext) { - getDashboardNotFoundError = m.ErrDashboardNotFound - sc.handlerFunc = DeleteDashboardAcl - sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec() - - Convey("Should not be able to delete non-existing dashboard", func() { - So(sc.resp.Code, ShouldEqual, 404) - }) - }) }) Convey("When user is org editor and has admin permission in the ACL", func() { loggedInUserScenarioWithRole("When calling GET on", "GET", "/api/dashboards/id/1/acl", "/api/dashboards/id/:dashboardId/acl", m.ROLE_EDITOR, func(sc *scenarioContext) { - mockResult = append(mockResult, &m.DashboardAclInfoDTO{Id: 6, OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}) + mockResult = append(mockResult, &m.DashboardAclInfoDTO{OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}) Convey("Should be able to access ACL", func() { sc.handlerFunc = GetDashboardAclList @@ -116,36 +106,6 @@ func TestDashboardAclApiEndpoint(t *testing.T) { }) }) - loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/dashboards/id/1/acl/1", "/api/dashboards/id/:dashboardId/acl/:aclId", m.ROLE_EDITOR, func(sc *scenarioContext) { - mockResult = append(mockResult, &m.DashboardAclInfoDTO{Id: 6, OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}) - - bus.AddHandler("test3", func(cmd *m.RemoveDashboardAclCommand) error { - return nil - }) - - Convey("Should be able to delete permission", func() { - sc.handlerFunc = DeleteDashboardAcl - sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec() - - So(sc.resp.Code, ShouldEqual, 200) - }) - }) - - loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/dashboards/id/1/acl/6", "/api/dashboards/id/:dashboardId/acl/:aclId", m.ROLE_EDITOR, func(sc *scenarioContext) { - mockResult = append(mockResult, &m.DashboardAclInfoDTO{Id: 6, OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}) - - bus.AddHandler("test3", func(cmd *m.RemoveDashboardAclCommand) error { - return nil - }) - - Convey("Should not be able to delete their own Admin permission", func() { - sc.handlerFunc = DeleteDashboardAcl - sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec() - - So(sc.resp.Code, ShouldEqual, 403) - }) - }) - Convey("Should not be able to downgrade their own Admin permission", func() { cmd := dtos.UpdateDashboardAclCommand{ Items: []dtos.DashboardAclUpdateItem{ @@ -154,7 +114,7 @@ func TestDashboardAclApiEndpoint(t *testing.T) { } postAclScenario("When calling POST on", "/api/dashboards/id/1/acl", "/api/dashboards/id/:dashboardId/acl", m.ROLE_EDITOR, cmd, func(sc *scenarioContext) { - mockResult = append(mockResult, &m.DashboardAclInfoDTO{Id: 6, OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}) + mockResult = append(mockResult, &m.DashboardAclInfoDTO{OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}) CallPostAcl(sc) So(sc.resp.Code, ShouldEqual, 403) @@ -170,34 +130,18 @@ func TestDashboardAclApiEndpoint(t *testing.T) { } postAclScenario("When calling POST on", "/api/dashboards/id/1/acl", "/api/dashboards/id/:dashboardId/acl", m.ROLE_EDITOR, cmd, func(sc *scenarioContext) { - mockResult = append(mockResult, &m.DashboardAclInfoDTO{Id: 6, OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}) + mockResult = append(mockResult, &m.DashboardAclInfoDTO{OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}) CallPostAcl(sc) So(sc.resp.Code, ShouldEqual, 200) }) }) - Convey("When user is a member of a team in the ACL with admin permission", func() { - loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/dashboards/id/1/acl/1", "/api/dashboards/id/:dashboardsId/acl/:aclId", m.ROLE_EDITOR, func(sc *scenarioContext) { - teamResp = append(teamResp, &m.Team{Id: 2, OrgId: 1, Name: "UG2"}) - - bus.AddHandler("test3", func(cmd *m.RemoveDashboardAclCommand) error { - return nil - }) - - Convey("Should be able to delete permission", func() { - sc.handlerFunc = DeleteDashboardAcl - sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec() - - So(sc.resp.Code, ShouldEqual, 200) - }) - }) - }) }) Convey("When user is org viewer and has edit permission in the ACL", func() { loggedInUserScenarioWithRole("When calling GET on", "GET", "/api/dashboards/id/1/acl", "/api/dashboards/id/:dashboardId/acl", m.ROLE_VIEWER, func(sc *scenarioContext) { - mockResult = append(mockResult, &m.DashboardAclInfoDTO{Id: 1, OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_EDIT}) + mockResult = append(mockResult, &m.DashboardAclInfoDTO{OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_EDIT}) // Getting the permissions is an Admin permission Convey("Should not be able to get list of permissions from ACL", func() { @@ -207,21 +151,6 @@ func TestDashboardAclApiEndpoint(t *testing.T) { So(sc.resp.Code, ShouldEqual, 403) }) }) - - loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/dashboards/id/1/acl/1", "/api/dashboards/id/:dashboardId/acl/:aclId", m.ROLE_VIEWER, func(sc *scenarioContext) { - mockResult = append(mockResult, &m.DashboardAclInfoDTO{Id: 1, OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_EDIT}) - - bus.AddHandler("test3", func(cmd *m.RemoveDashboardAclCommand) error { - return nil - }) - - Convey("Should be not be able to delete permission", func() { - sc.handlerFunc = DeleteDashboardAcl - sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec() - - So(sc.resp.Code, ShouldEqual, 403) - }) - }) }) Convey("When user is org editor and not in the ACL", func() { @@ -234,20 +163,6 @@ func TestDashboardAclApiEndpoint(t *testing.T) { So(sc.resp.Code, ShouldEqual, 403) }) }) - - loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/dashboards/id/1/acl/user/1", "/api/dashboards/id/:dashboardsId/acl/user/:userId", m.ROLE_EDITOR, func(sc *scenarioContext) { - mockResult = append(mockResult, &m.DashboardAclInfoDTO{Id: 1, OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_VIEW}) - bus.AddHandler("test3", func(cmd *m.RemoveDashboardAclCommand) error { - return nil - }) - - Convey("Should be not be able to delete permission", func() { - sc.handlerFunc = DeleteDashboardAcl - sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec() - - So(sc.resp.Code, ShouldEqual, 403) - }) - }) }) }) } @@ -257,7 +172,6 @@ func transformDashboardAclsToDTOs(acls []*m.DashboardAclInfoDTO) []*m.DashboardA for _, acl := range acls { dto := &m.DashboardAclInfoDTO{ - Id: acl.Id, OrgId: acl.OrgId, DashboardId: acl.DashboardId, Permission: acl.Permission, diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index e80b3cad4dc..4a45c561d57 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -431,7 +431,7 @@ func TestDashboardApiEndpoint(t *testing.T) { role := m.ROLE_VIEWER mockResult := []*m.DashboardAclInfoDTO{ - {Id: 1, OrgId: 1, DashboardId: 2, UserId: 1, Permission: m.PERMISSION_EDIT}, + {OrgId: 1, DashboardId: 2, UserId: 1, Permission: m.PERMISSION_EDIT}, } bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { @@ -505,7 +505,7 @@ func TestDashboardApiEndpoint(t *testing.T) { setting.ViewersCanEdit = true mockResult := []*m.DashboardAclInfoDTO{ - {Id: 1, OrgId: 1, DashboardId: 2, UserId: 1, Permission: m.PERMISSION_VIEW}, + {OrgId: 1, DashboardId: 2, UserId: 1, Permission: m.PERMISSION_VIEW}, } bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { @@ -564,7 +564,7 @@ func TestDashboardApiEndpoint(t *testing.T) { role := m.ROLE_VIEWER mockResult := []*m.DashboardAclInfoDTO{ - {Id: 1, OrgId: 1, DashboardId: 2, UserId: 1, Permission: m.PERMISSION_ADMIN}, + {OrgId: 1, DashboardId: 2, UserId: 1, Permission: m.PERMISSION_ADMIN}, } bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { @@ -637,7 +637,7 @@ func TestDashboardApiEndpoint(t *testing.T) { role := m.ROLE_EDITOR mockResult := []*m.DashboardAclInfoDTO{ - {Id: 1, OrgId: 1, DashboardId: 2, UserId: 1, Permission: m.PERMISSION_VIEW}, + {OrgId: 1, DashboardId: 2, UserId: 1, Permission: m.PERMISSION_VIEW}, } bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { diff --git a/pkg/models/dashboard_acl.go b/pkg/models/dashboard_acl.go index 933487650e3..202b519207d 100644 --- a/pkg/models/dashboard_acl.go +++ b/pkg/models/dashboard_acl.go @@ -44,7 +44,6 @@ type DashboardAcl struct { } type DashboardAclInfoDTO struct { - Id int64 `json:"id"` OrgId int64 `json:"-"` DashboardId int64 `json:"dashboardId"` @@ -75,21 +74,6 @@ type UpdateDashboardAclCommand struct { Items []*DashboardAcl } -type SetDashboardAclCommand struct { - DashboardId int64 - OrgId int64 - UserId int64 - TeamId int64 - Permission PermissionType - - Result DashboardAcl -} - -type RemoveDashboardAclCommand struct { - AclId int64 - OrgId int64 -} - // // QUERIES // diff --git a/pkg/services/guardian/guardian.go b/pkg/services/guardian/guardian.go index b448561494d..05795b7f2df 100644 --- a/pkg/services/guardian/guardian.go +++ b/pkg/services/guardian/guardian.go @@ -106,26 +106,6 @@ func (g *DashboardGuardian) checkAcl(permission m.PermissionType, acl []*m.Dashb return false, nil } -func (g *DashboardGuardian) CheckPermissionBeforeRemove(permission m.PermissionType, aclIdToRemove int64) (bool, error) { - if g.user.OrgRole == m.ROLE_ADMIN { - return true, nil - } - - acl, err := g.GetAcl() - if err != nil { - return false, err - } - - for i, p := range acl { - if p.Id == aclIdToRemove { - acl = append(acl[:i], acl[i+1:]...) - break - } - } - - return g.checkAcl(permission, acl) -} - func (g *DashboardGuardian) CheckPermissionBeforeUpdate(permission m.PermissionType, updatePermissions []*m.DashboardAcl) (bool, error) { if g.user.OrgRole == m.ROLE_ADMIN { return true, nil diff --git a/pkg/services/sqlstore/dashboard.go b/pkg/services/sqlstore/dashboard.go index f3fd81ebbe2..42c83da8810 100644 --- a/pkg/services/sqlstore/dashboard.go +++ b/pkg/services/sqlstore/dashboard.go @@ -79,11 +79,6 @@ func saveDashboard(sess *DBSession, cmd *m.SaveDashboardCommand) error { dash.Data.Set("uid", uid) } - err = setHasAcl(sess, dash) - if err != nil { - return err - } - parentVersion := dash.Version affectedRows := int64(0) @@ -100,7 +95,7 @@ func saveDashboard(sess *DBSession, cmd *m.SaveDashboardCommand) error { dash.Updated = cmd.UpdatedAt } - affectedRows, err = sess.MustCols("folder_id", "has_acl").ID(dash.Id).Update(dash) + affectedRows, err = sess.MustCols("folder_id").ID(dash.Id).Update(dash) } if err != nil { @@ -233,31 +228,6 @@ func generateNewDashboardUid(sess *DBSession, orgId int64) (string, error) { return "", m.ErrDashboardFailedGenerateUniqueUid } -func setHasAcl(sess *DBSession, dash *m.Dashboard) error { - // check if parent has acl - if dash.FolderId > 0 { - var parent m.Dashboard - if hasParent, err := sess.Where("folder_id=?", dash.FolderId).Get(&parent); err != nil { - return err - } else if hasParent && parent.HasAcl { - dash.HasAcl = true - } - } - - // check if dash has its own acl - if dash.Id > 0 { - if res, err := sess.Query("SELECT 1 from dashboard_acl WHERE dashboard_id =?", dash.Id); err != nil { - return err - } else { - if len(res) > 0 { - dash.HasAcl = true - } - } - } - - return nil -} - func GetDashboard(query *m.GetDashboardQuery) error { dashboard := m.Dashboard{Slug: query.Slug, OrgId: query.OrgId, Id: query.Id, Uid: query.Uid} has, err := x.Get(&dashboard) diff --git a/pkg/services/sqlstore/dashboard_acl.go b/pkg/services/sqlstore/dashboard_acl.go index 829182a8195..ae91d1d41f3 100644 --- a/pkg/services/sqlstore/dashboard_acl.go +++ b/pkg/services/sqlstore/dashboard_acl.go @@ -1,17 +1,12 @@ package sqlstore import ( - "fmt" - "time" - "github.com/grafana/grafana/pkg/bus" m "github.com/grafana/grafana/pkg/models" ) func init() { - bus.AddHandler("sql", SetDashboardAcl) bus.AddHandler("sql", UpdateDashboardAcl) - bus.AddHandler("sql", RemoveDashboardAcl) bus.AddHandler("sql", GetDashboardAclInfoList) } @@ -24,7 +19,7 @@ func UpdateDashboardAcl(cmd *m.UpdateDashboardAclCommand) error { } for _, item := range cmd.Items { - if item.UserId == 0 && item.TeamId == 0 && !item.Role.IsValid() { + if item.UserId == 0 && item.TeamId == 0 && (item.Role == nil || !item.Role.IsValid()) { return m.ErrDashboardAclInfoMissing } @@ -40,92 +35,13 @@ func UpdateDashboardAcl(cmd *m.UpdateDashboardAclCommand) error { // Update dashboard HasAcl flag dashboard := m.Dashboard{HasAcl: true} - if _, err := sess.Cols("has_acl").Where("id=? OR folder_id=?", cmd.DashboardId, cmd.DashboardId).Update(&dashboard); err != nil { + if _, err := sess.Cols("has_acl").Where("id=?", cmd.DashboardId).Update(&dashboard); err != nil { return err } return nil }) } -func SetDashboardAcl(cmd *m.SetDashboardAclCommand) error { - return inTransaction(func(sess *DBSession) error { - if cmd.UserId == 0 && cmd.TeamId == 0 { - return m.ErrDashboardAclInfoMissing - } - - if cmd.DashboardId == 0 { - return m.ErrDashboardPermissionDashboardEmpty - } - - if res, err := sess.Query("SELECT 1 from "+dialect.Quote("dashboard_acl")+" WHERE dashboard_id =? and (team_id=? or user_id=?)", cmd.DashboardId, cmd.TeamId, cmd.UserId); err != nil { - return err - } else if len(res) == 1 { - - entity := m.DashboardAcl{ - Permission: cmd.Permission, - Updated: time.Now(), - } - - if _, err := sess.Cols("updated", "permission").Where("dashboard_id =? and (team_id=? or user_id=?)", cmd.DashboardId, cmd.TeamId, cmd.UserId).Update(&entity); err != nil { - return err - } - - return nil - } - - entity := m.DashboardAcl{ - OrgId: cmd.OrgId, - TeamId: cmd.TeamId, - UserId: cmd.UserId, - Created: time.Now(), - Updated: time.Now(), - DashboardId: cmd.DashboardId, - Permission: cmd.Permission, - } - - cols := []string{"org_id", "created", "updated", "dashboard_id", "permission"} - - if cmd.UserId != 0 { - cols = append(cols, "user_id") - } - - if cmd.TeamId != 0 { - cols = append(cols, "team_id") - } - - _, err := sess.Cols(cols...).Insert(&entity) - if err != nil { - return err - } - - cmd.Result = entity - - // Update dashboard HasAcl flag - dashboard := m.Dashboard{ - HasAcl: true, - } - - if _, err := sess.Cols("has_acl").Where("id=? OR folder_id=?", cmd.DashboardId, cmd.DashboardId).Update(&dashboard); err != nil { - return err - } - - return nil - }) -} - -// RemoveDashboardAcl removes a specified permission from the dashboard acl -func RemoveDashboardAcl(cmd *m.RemoveDashboardAclCommand) error { - return inTransaction(func(sess *DBSession) error { - var rawSQL = "DELETE FROM " + dialect.Quote("dashboard_acl") + " WHERE org_id =? and id=?" - _, err := sess.Exec(rawSQL, cmd.OrgId, cmd.AclId) - if err != nil { - return err - } - - return err - }) -} - // GetDashboardAclInfoList returns a list of permissions for a dashboard. They can be fetched from three // different places. // 1) Permissions for the dashboard @@ -134,6 +50,8 @@ func RemoveDashboardAcl(cmd *m.RemoveDashboardAclCommand) error { func GetDashboardAclInfoList(query *m.GetDashboardAclInfoListQuery) error { var err error + falseStr := dialect.BooleanStr(false) + if query.DashboardId == 0 { sql := `SELECT da.id, @@ -151,18 +69,13 @@ func GetDashboardAclInfoList(query *m.GetDashboardAclInfoListQuery) error { '' as title, '' as slug, '' as uid,` + - dialect.BooleanStr(false) + ` AS is_folder + falseStr + ` AS is_folder FROM dashboard_acl as da WHERE da.dashboard_id = -1` query.Result = make([]*m.DashboardAclInfoDTO, 0) err = x.SQL(sql).Find(&query.Result) } else { - dashboardFilter := fmt.Sprintf(`IN ( - SELECT %d - UNION - SELECT folder_id from dashboard where id = %d - )`, query.DashboardId, query.DashboardId) rawSQL := ` -- get permissions for the dashboard and its parent folder @@ -183,41 +96,21 @@ func GetDashboardAclInfoList(query *m.GetDashboardAclInfoListQuery) error { d.slug, d.uid, d.is_folder - FROM` + dialect.Quote("dashboard_acl") + ` as da - LEFT OUTER JOIN ` + dialect.Quote("user") + ` AS u ON u.id = da.user_id - LEFT OUTER JOIN team ug on ug.id = da.team_id - LEFT OUTER JOIN dashboard d on da.dashboard_id = d.id - WHERE dashboard_id ` + dashboardFilter + ` AND da.org_id = ? - - -- Also include default permissions if folder or dashboard field "has_acl" is false - - UNION - SELECT - da.id, - da.org_id, - da.dashboard_id, - da.user_id, - da.team_id, - da.permission, - da.role, - da.created, - da.updated, - '' as user_login, - '' as user_email, - '' as team, - folder.title, - folder.slug, - folder.uid, - folder.is_folder - FROM dashboard_acl as da, - dashboard as dash - LEFT OUTER JOIN dashboard folder on dash.folder_id = folder.id - WHERE - dash.id = ? AND ( - dash.has_acl = ` + dialect.BooleanStr(false) + ` or - folder.has_acl = ` + dialect.BooleanStr(false) + ` - ) AND - da.dashboard_id = -1 + FROM dashboard as d + LEFT JOIN dashboard folder on folder.id = d.folder_id + LEFT JOIN dashboard_acl AS da ON + da.dashboard_id = d.id OR + da.dashboard_id = d.folder_id OR + ( + -- include default permissions --> + da.org_id = -1 AND ( + (folder.id IS NOT NULL AND folder.has_acl = ` + falseStr + `) OR + (folder.id IS NULL AND d.has_acl = ` + falseStr + `) + ) + ) + LEFT JOIN ` + dialect.Quote("user") + ` AS u ON u.id = da.user_id + LEFT JOIN team ug on ug.id = da.team_id + WHERE d.org_id = ? AND d.id = ? AND da.id IS NOT NULL ORDER BY 1 ASC ` diff --git a/pkg/services/sqlstore/dashboard_acl_test.go b/pkg/services/sqlstore/dashboard_acl_test.go index 8b712c73ece..8fbb9c0d813 100644 --- a/pkg/services/sqlstore/dashboard_acl_test.go +++ b/pkg/services/sqlstore/dashboard_acl_test.go @@ -17,7 +17,7 @@ func TestDashboardAclDataAccess(t *testing.T) { childDash := insertTestDashboard("2 test dash", 1, savedFolder.Id, false, "prod", "webapp") Convey("When adding dashboard permission with userId and teamId set to 0", func() { - err := SetDashboardAcl(&m.SetDashboardAclCommand{ + err := testHelperUpdateDashboardAcl(savedFolder.Id, m.DashboardAcl{ OrgId: 1, DashboardId: savedFolder.Id, Permission: m.PERMISSION_EDIT, @@ -41,8 +41,25 @@ func TestDashboardAclDataAccess(t *testing.T) { }) }) + Convey("Given dashboard folder with removed default permissions", func() { + err := UpdateDashboardAcl(&m.UpdateDashboardAclCommand{ + DashboardId: savedFolder.Id, + Items: []*m.DashboardAcl{}, + }) + So(err, ShouldBeNil) + + Convey("When reading dashboard acl should return no acl items", func() { + query := m.GetDashboardAclInfoListQuery{DashboardId: childDash.Id, OrgId: 1} + + err := GetDashboardAclInfoList(&query) + So(err, ShouldBeNil) + + So(len(query.Result), ShouldEqual, 0) + }) + }) + Convey("Given dashboard folder permission", func() { - err := SetDashboardAcl(&m.SetDashboardAclCommand{ + err := testHelperUpdateDashboardAcl(savedFolder.Id, m.DashboardAcl{ OrgId: 1, UserId: currentUser.Id, DashboardId: savedFolder.Id, @@ -61,7 +78,7 @@ func TestDashboardAclDataAccess(t *testing.T) { }) Convey("Given child dashboard permission", func() { - err := SetDashboardAcl(&m.SetDashboardAclCommand{ + err := testHelperUpdateDashboardAcl(childDash.Id, m.DashboardAcl{ OrgId: 1, UserId: currentUser.Id, DashboardId: childDash.Id, @@ -83,7 +100,7 @@ func TestDashboardAclDataAccess(t *testing.T) { }) Convey("Given child dashboard permission in folder with no permissions", func() { - err := SetDashboardAcl(&m.SetDashboardAclCommand{ + err := testHelperUpdateDashboardAcl(childDash.Id, m.DashboardAcl{ OrgId: 1, UserId: currentUser.Id, DashboardId: childDash.Id, @@ -108,17 +125,12 @@ func TestDashboardAclDataAccess(t *testing.T) { }) Convey("Should be able to add dashboard permission", func() { - setDashAclCmd := m.SetDashboardAclCommand{ + err := testHelperUpdateDashboardAcl(savedFolder.Id, m.DashboardAcl{ OrgId: 1, UserId: currentUser.Id, DashboardId: savedFolder.Id, Permission: m.PERMISSION_EDIT, - } - - err := SetDashboardAcl(&setDashAclCmd) - So(err, ShouldBeNil) - - So(setDashAclCmd.Result.Id, ShouldEqual, 3) + }) q1 := &m.GetDashboardAclInfoListQuery{DashboardId: savedFolder.Id, OrgId: 1} err = GetDashboardAclInfoList(q1) @@ -130,42 +142,9 @@ func TestDashboardAclDataAccess(t *testing.T) { So(q1.Result[0].UserId, ShouldEqual, currentUser.Id) So(q1.Result[0].UserLogin, ShouldEqual, currentUser.Login) So(q1.Result[0].UserEmail, ShouldEqual, currentUser.Email) - So(q1.Result[0].Id, ShouldEqual, setDashAclCmd.Result.Id) - - Convey("Should update hasAcl field to true for dashboard folder and its children", func() { - q2 := &m.GetDashboardsQuery{DashboardIds: []int64{savedFolder.Id, childDash.Id}} - err := GetDashboards(q2) - So(err, ShouldBeNil) - So(q2.Result[0].HasAcl, ShouldBeTrue) - So(q2.Result[1].HasAcl, ShouldBeTrue) - }) - - Convey("Should be able to update an existing permission", func() { - err := SetDashboardAcl(&m.SetDashboardAclCommand{ - OrgId: 1, - UserId: 1, - DashboardId: savedFolder.Id, - Permission: m.PERMISSION_ADMIN, - }) - - So(err, ShouldBeNil) - - q3 := &m.GetDashboardAclInfoListQuery{DashboardId: savedFolder.Id, OrgId: 1} - err = GetDashboardAclInfoList(q3) - So(err, ShouldBeNil) - So(len(q3.Result), ShouldEqual, 1) - So(q3.Result[0].DashboardId, ShouldEqual, savedFolder.Id) - So(q3.Result[0].Permission, ShouldEqual, m.PERMISSION_ADMIN) - So(q3.Result[0].UserId, ShouldEqual, 1) - - }) Convey("Should be able to delete an existing permission", func() { - err := RemoveDashboardAcl(&m.RemoveDashboardAclCommand{ - OrgId: 1, - AclId: setDashAclCmd.Result.Id, - }) - + err := testHelperUpdateDashboardAcl(savedFolder.Id) So(err, ShouldBeNil) q3 := &m.GetDashboardAclInfoListQuery{DashboardId: savedFolder.Id, OrgId: 1} @@ -181,14 +160,12 @@ func TestDashboardAclDataAccess(t *testing.T) { So(err, ShouldBeNil) Convey("Should be able to add a user permission for a team", func() { - setDashAclCmd := m.SetDashboardAclCommand{ + err := testHelperUpdateDashboardAcl(savedFolder.Id, m.DashboardAcl{ OrgId: 1, TeamId: group1.Result.Id, DashboardId: savedFolder.Id, Permission: m.PERMISSION_EDIT, - } - - err := SetDashboardAcl(&setDashAclCmd) + }) So(err, ShouldBeNil) q1 := &m.GetDashboardAclInfoListQuery{DashboardId: savedFolder.Id, OrgId: 1} @@ -197,23 +174,10 @@ func TestDashboardAclDataAccess(t *testing.T) { So(q1.Result[0].DashboardId, ShouldEqual, savedFolder.Id) So(q1.Result[0].Permission, ShouldEqual, m.PERMISSION_EDIT) So(q1.Result[0].TeamId, ShouldEqual, group1.Result.Id) - - Convey("Should be able to delete an existing permission for a team", func() { - err := RemoveDashboardAcl(&m.RemoveDashboardAclCommand{ - OrgId: 1, - AclId: setDashAclCmd.Result.Id, - }) - - So(err, ShouldBeNil) - q3 := &m.GetDashboardAclInfoListQuery{DashboardId: savedFolder.Id, OrgId: 1} - err = GetDashboardAclInfoList(q3) - So(err, ShouldBeNil) - So(len(q3.Result), ShouldEqual, 0) - }) }) Convey("Should be able to update an existing permission for a team", func() { - err := SetDashboardAcl(&m.SetDashboardAclCommand{ + err := testHelperUpdateDashboardAcl(savedFolder.Id, m.DashboardAcl{ OrgId: 1, TeamId: group1.Result.Id, DashboardId: savedFolder.Id, @@ -229,7 +193,6 @@ func TestDashboardAclDataAccess(t *testing.T) { So(q3.Result[0].Permission, ShouldEqual, m.PERMISSION_ADMIN) So(q3.Result[0].TeamId, ShouldEqual, group1.Result.Id) }) - }) }) diff --git a/pkg/services/sqlstore/dashboard_folder_test.go b/pkg/services/sqlstore/dashboard_folder_test.go index b32a4dfed1d..40d6cf5bcb2 100644 --- a/pkg/services/sqlstore/dashboard_folder_test.go +++ b/pkg/services/sqlstore/dashboard_folder_test.go @@ -41,7 +41,7 @@ func TestDashboardFolderDataAccess(t *testing.T) { Convey("and acl is set for dashboard folder", func() { var otherUser int64 = 999 - updateTestDashboardWithAcl(folder.Id, otherUser, m.PERMISSION_EDIT) + testHelperUpdateDashboardAcl(folder.Id, m.DashboardAcl{DashboardId: folder.Id, OrgId: 1, UserId: otherUser, Permission: m.PERMISSION_EDIT}) Convey("should not return folder", func() { query := &search.FindPersistedDashboardsQuery{ @@ -55,7 +55,7 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) Convey("when the user is given permission", func() { - updateTestDashboardWithAcl(folder.Id, currentUser.Id, m.PERMISSION_EDIT) + testHelperUpdateDashboardAcl(folder.Id, m.DashboardAcl{DashboardId: folder.Id, OrgId: 1, UserId: currentUser.Id, Permission: m.PERMISSION_EDIT}) Convey("should be able to access folder", func() { query := &search.FindPersistedDashboardsQuery{ @@ -93,9 +93,8 @@ func TestDashboardFolderDataAccess(t *testing.T) { Convey("and acl is set for dashboard child and folder has all permissions removed", func() { var otherUser int64 = 999 - aclId := updateTestDashboardWithAcl(folder.Id, otherUser, m.PERMISSION_EDIT) - removeAcl(aclId) - updateTestDashboardWithAcl(childDash.Id, otherUser, m.PERMISSION_EDIT) + testHelperUpdateDashboardAcl(folder.Id) + testHelperUpdateDashboardAcl(childDash.Id, m.DashboardAcl{DashboardId: folder.Id, OrgId: 1, UserId: otherUser, Permission: m.PERMISSION_EDIT}) Convey("should not return folder or child", func() { query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1, OrgRole: m.ROLE_VIEWER}, OrgId: 1, DashboardIds: []int64{folder.Id, childDash.Id, dashInRoot.Id}} @@ -106,7 +105,7 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) Convey("when the user is given permission to child", func() { - updateTestDashboardWithAcl(childDash.Id, currentUser.Id, m.PERMISSION_EDIT) + testHelperUpdateDashboardAcl(childDash.Id, m.DashboardAcl{DashboardId: childDash.Id, OrgId: 1, UserId: currentUser.Id, Permission: m.PERMISSION_EDIT}) Convey("should be able to search for child dashboard but not folder", func() { query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1, OrgRole: m.ROLE_VIEWER}, OrgId: 1, DashboardIds: []int64{folder.Id, childDash.Id, dashInRoot.Id}} @@ -165,11 +164,10 @@ func TestDashboardFolderDataAccess(t *testing.T) { Convey("and acl is set for one dashboard folder", func() { var otherUser int64 = 999 - updateTestDashboardWithAcl(folder1.Id, otherUser, m.PERMISSION_EDIT) + testHelperUpdateDashboardAcl(folder1.Id, m.DashboardAcl{DashboardId: folder1.Id, OrgId: 1, UserId: otherUser, Permission: m.PERMISSION_EDIT}) Convey("and a dashboard is moved from folder without acl to the folder with an acl", func() { - movedDash := moveDashboard(1, childDash2.Data, folder1.Id) - So(movedDash.HasAcl, ShouldBeTrue) + moveDashboard(1, childDash2.Data, folder1.Id) Convey("should not return folder with acl or its children", func() { query := &search.FindPersistedDashboardsQuery{ @@ -184,9 +182,7 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) }) Convey("and a dashboard is moved from folder with acl to the folder without an acl", func() { - - movedDash := moveDashboard(1, childDash1.Data, folder2.Id) - So(movedDash.HasAcl, ShouldBeFalse) + moveDashboard(1, childDash1.Data, folder2.Id) Convey("should return folder without acl and its children", func() { query := &search.FindPersistedDashboardsQuery{ @@ -205,9 +201,8 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) Convey("and a dashboard with an acl is moved to the folder without an acl", func() { - updateTestDashboardWithAcl(childDash1.Id, otherUser, m.PERMISSION_EDIT) - movedDash := moveDashboard(1, childDash1.Data, folder2.Id) - So(movedDash.HasAcl, ShouldBeTrue) + testHelperUpdateDashboardAcl(childDash1.Id, m.DashboardAcl{DashboardId: childDash1.Id, OrgId: 1, UserId: otherUser, Permission: m.PERMISSION_EDIT}) + moveDashboard(1, childDash1.Data, folder2.Id) Convey("should return folder without acl but not the dashboard with acl", func() { query := &search.FindPersistedDashboardsQuery{ @@ -308,7 +303,7 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) Convey("Should have write access to one dashboard folder if default role changed to view for one folder", func() { - updateTestDashboardWithAcl(folder1.Id, editorUser.Id, m.PERMISSION_VIEW) + testHelperUpdateDashboardAcl(folder1.Id, m.DashboardAcl{DashboardId: folder1.Id, OrgId: 1, UserId: editorUser.Id, Permission: m.PERMISSION_VIEW}) err := SearchDashboards(&query) So(err, ShouldBeNil) @@ -352,7 +347,7 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) Convey("Should be able to get one dashboard folder if default role changed to edit for one folder", func() { - updateTestDashboardWithAcl(folder1.Id, viewerUser.Id, m.PERMISSION_EDIT) + testHelperUpdateDashboardAcl(folder1.Id, m.DashboardAcl{DashboardId: folder1.Id, OrgId: 1, UserId: viewerUser.Id, Permission: m.PERMISSION_EDIT}) err := SearchDashboards(&query) So(err, ShouldBeNil) diff --git a/pkg/services/sqlstore/dashboard_test.go b/pkg/services/sqlstore/dashboard_test.go index de7cdf19927..7de4c5f5701 100644 --- a/pkg/services/sqlstore/dashboard_test.go +++ b/pkg/services/sqlstore/dashboard_test.go @@ -663,25 +663,6 @@ func createUser(name string, role string, isAdmin bool) m.User { return currentUserCmd.Result } -func updateTestDashboardWithAcl(dashId int64, userId int64, permissions m.PermissionType) int64 { - cmd := &m.SetDashboardAclCommand{ - OrgId: 1, - UserId: userId, - DashboardId: dashId, - Permission: permissions, - } - - err := SetDashboardAcl(cmd) - So(err, ShouldBeNil) - - return cmd.Result.Id -} - -func removeAcl(aclId int64) { - err := RemoveDashboardAcl(&m.RemoveDashboardAclCommand{AclId: aclId, OrgId: 1}) - So(err, ShouldBeNil) -} - func moveDashboard(orgId int64, dashboard *simplejson.Json, newFolderId int64) *m.Dashboard { cmd := m.SaveDashboardCommand{ OrgId: orgId, diff --git a/pkg/services/sqlstore/org_test.go b/pkg/services/sqlstore/org_test.go index 5322dfd4748..c57d15a48d5 100644 --- a/pkg/services/sqlstore/org_test.go +++ b/pkg/services/sqlstore/org_test.go @@ -199,10 +199,13 @@ func TestAccountDataAccess(t *testing.T) { So(err, ShouldBeNil) So(len(query.Result), ShouldEqual, 3) - err = SetDashboardAcl(&m.SetDashboardAclCommand{DashboardId: 1, OrgId: ac1.OrgId, UserId: ac3.Id, Permission: m.PERMISSION_EDIT}) + dash1 := insertTestDashboard("1 test dash", ac1.OrgId, 0, false, "prod", "webapp") + dash2 := insertTestDashboard("2 test dash", ac3.OrgId, 0, false, "prod", "webapp") + + err = testHelperUpdateDashboardAcl(dash1.Id, m.DashboardAcl{DashboardId: dash1.Id, OrgId: ac1.OrgId, UserId: ac3.Id, Permission: m.PERMISSION_EDIT}) So(err, ShouldBeNil) - err = SetDashboardAcl(&m.SetDashboardAclCommand{DashboardId: 2, OrgId: ac3.OrgId, UserId: ac3.Id, Permission: m.PERMISSION_EDIT}) + err = testHelperUpdateDashboardAcl(dash2.Id, m.DashboardAcl{DashboardId: dash2.Id, OrgId: ac3.OrgId, UserId: ac3.Id, Permission: m.PERMISSION_EDIT}) So(err, ShouldBeNil) Convey("When org user is deleted", func() { @@ -234,3 +237,11 @@ func TestAccountDataAccess(t *testing.T) { }) }) } + +func testHelperUpdateDashboardAcl(dashboardId int64, items ...m.DashboardAcl) error { + cmd := m.UpdateDashboardAclCommand{DashboardId: dashboardId} + for _, item := range items { + cmd.Items = append(cmd.Items, &item) + } + return UpdateDashboardAcl(&cmd) +} diff --git a/pkg/services/sqlstore/team_test.go b/pkg/services/sqlstore/team_test.go index bebe59f4238..fb76c3fa9d6 100644 --- a/pkg/services/sqlstore/team_test.go +++ b/pkg/services/sqlstore/team_test.go @@ -99,7 +99,7 @@ func TestTeamCommandsAndQueries(t *testing.T) { So(err, ShouldBeNil) err = AddTeamMember(&m.AddTeamMemberCommand{OrgId: testOrgId, TeamId: groupId, UserId: userIds[2]}) So(err, ShouldBeNil) - err = SetDashboardAcl(&m.SetDashboardAclCommand{DashboardId: 1, OrgId: testOrgId, Permission: m.PERMISSION_EDIT, TeamId: groupId}) + err = testHelperUpdateDashboardAcl(1, m.DashboardAcl{DashboardId: 1, OrgId: testOrgId, Permission: m.PERMISSION_EDIT, TeamId: groupId}) err = DeleteTeam(&m.DeleteTeamCommand{OrgId: testOrgId, Id: groupId}) So(err, ShouldBeNil) diff --git a/pkg/services/sqlstore/user_test.go b/pkg/services/sqlstore/user_test.go index a65b7226eb6..2830733c96a 100644 --- a/pkg/services/sqlstore/user_test.go +++ b/pkg/services/sqlstore/user_test.go @@ -99,7 +99,7 @@ func TestUserDataAccess(t *testing.T) { err = AddOrgUser(&m.AddOrgUserCommand{LoginOrEmail: users[0].Login, Role: m.ROLE_VIEWER, OrgId: users[0].OrgId}) So(err, ShouldBeNil) - err = SetDashboardAcl(&m.SetDashboardAclCommand{DashboardId: 1, OrgId: users[0].OrgId, UserId: users[0].Id, Permission: m.PERMISSION_EDIT}) + testHelperUpdateDashboardAcl(1, m.DashboardAcl{DashboardId: 1, OrgId: users[0].OrgId, UserId: users[0].Id, Permission: m.PERMISSION_EDIT}) So(err, ShouldBeNil) err = SavePreferences(&m.SavePreferencesCommand{UserId: users[0].Id, OrgId: users[0].OrgId, HomeDashboardId: 1, Theme: "dark"}) diff --git a/public/app/stores/PermissionsStore/PermissionsStoreItem.ts b/public/app/stores/PermissionsStore/PermissionsStoreItem.ts index 74769891256..92dca0220ca 100644 --- a/public/app/stores/PermissionsStore/PermissionsStoreItem.ts +++ b/public/app/stores/PermissionsStore/PermissionsStoreItem.ts @@ -1,9 +1,8 @@ -import { types } from 'mobx-state-tree'; +import { types } from 'mobx-state-tree'; export const PermissionsStoreItem = types .model('PermissionsStoreItem', { dashboardId: types.optional(types.number, -1), - id: types.maybe(types.number), permission: types.number, permissionName: types.maybe(types.string), role: types.maybe(types.string),