From 955dfcc8fe364bc7e3ff8ebbaaaab3ac12fc8cfc Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Mon, 26 Feb 2018 19:12:01 +0100 Subject: [PATCH 1/6] dashboards: don't allow override of permissions with a lower precedence If a dashboard inherits permissions from a folder, don't allow same permission to be added to the dashboard with a lower permission. Add backend validation so that you cannot add same permission to folder/dashboard, for example same user/team with different permissions --- pkg/services/guardian/guardian.go | 50 +- pkg/services/guardian/guardian_test.go | 659 +++++++++++++++++++++++++ 2 files changed, 702 insertions(+), 7 deletions(-) create mode 100644 pkg/services/guardian/guardian_test.go diff --git a/pkg/services/guardian/guardian.go b/pkg/services/guardian/guardian.go index 23d43a53f35..972e87de39b 100644 --- a/pkg/services/guardian/guardian.go +++ b/pkg/services/guardian/guardian.go @@ -1,12 +1,19 @@ package guardian import ( + "errors" + "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/log" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/setting" ) +var ( + ErrGuardianDuplicatePermission = errors.New("You cannot add multiple permissions for a user, team or role") + ErrGuardianOverrideLowerPresedence = errors.New("You cannot override a permission with a lower presedence permission") +) + // DashboardGuardian to be used for guard against operations without access on dashboard and acl type DashboardGuardian interface { CanSave() (bool, error) @@ -119,14 +126,41 @@ func (g *dashboardGuardianImpl) checkAcl(permission m.PermissionType, acl []*m.D } func (g *dashboardGuardianImpl) CheckPermissionBeforeUpdate(permission m.PermissionType, updatePermissions []*m.DashboardAcl) (bool, error) { - if g.user.OrgRole == m.ROLE_ADMIN { - return true, nil - } - acl := []*m.DashboardAclInfoDTO{} for _, p := range updatePermissions { - acl = append(acl, &m.DashboardAclInfoDTO{UserId: p.UserId, TeamId: p.TeamId, Role: p.Role, Permission: p.Permission}) + for _, a := range acl { + if (a.UserId <= 0 && a.TeamId <= 0 && a.UserId == p.UserId && a.TeamId == p.TeamId && a.Role == p.Role) || + (a.UserId > 0 && a.UserId == p.UserId) || + (a.TeamId > 0 && a.TeamId == p.TeamId) { + return false, ErrGuardianDuplicatePermission + } + } + + acl = append(acl, &m.DashboardAclInfoDTO{DashboardId: p.DashboardId, UserId: p.UserId, TeamId: p.TeamId, Role: p.Role, Permission: p.Permission}) + } + + existingPermissions, err := g.GetAcl() + if err != nil { + return false, err + } + + for _, a := range acl { + for _, existingPerm := range existingPermissions { + if a.DashboardId == existingPerm.DashboardId { + continue + } + + if (a.UserId <= 0 && a.TeamId <= 0 && a.UserId == existingPerm.UserId && a.TeamId == existingPerm.TeamId && *a.Role == *existingPerm.Role && a.Permission <= existingPerm.Permission) || + (a.UserId > 0 && a.UserId == existingPerm.UserId && a.Permission <= existingPerm.Permission) || + (a.TeamId > 0 && a.TeamId == existingPerm.TeamId && a.Permission <= existingPerm.Permission) { + return false, ErrGuardianOverrideLowerPresedence + } + } + } + + if g.user.OrgRole == m.ROLE_ADMIN { + return true, nil } return g.checkAcl(permission, acl) @@ -169,6 +203,8 @@ type FakeDashboardGuardian struct { CanAdminValue bool HasPermissionValue bool CheckPermissionBeforeUpdateValue bool + CheckPermissionBeforeUpdateError error + GetAclValue []*m.DashboardAclInfoDTO } func (g *FakeDashboardGuardian) CanSave() (bool, error) { @@ -192,11 +228,11 @@ func (g *FakeDashboardGuardian) HasPermission(permission m.PermissionType) (bool } func (g *FakeDashboardGuardian) CheckPermissionBeforeUpdate(permission m.PermissionType, updatePermissions []*m.DashboardAcl) (bool, error) { - return g.CheckPermissionBeforeUpdateValue, nil + return g.CheckPermissionBeforeUpdateValue, g.CheckPermissionBeforeUpdateError } func (g *FakeDashboardGuardian) GetAcl() ([]*m.DashboardAclInfoDTO, error) { - return nil, nil + return g.GetAclValue, nil } func MockDashboardGuardian(mock *FakeDashboardGuardian) { diff --git a/pkg/services/guardian/guardian_test.go b/pkg/services/guardian/guardian_test.go new file mode 100644 index 00000000000..d420d1add97 --- /dev/null +++ b/pkg/services/guardian/guardian_test.go @@ -0,0 +1,659 @@ +package guardian + +import ( + "fmt" + "testing" + + "github.com/grafana/grafana/pkg/bus" + + m "github.com/grafana/grafana/pkg/models" + . "github.com/smartystreets/goconvey/convey" +) + +func TestGuardian(t *testing.T) { + Convey("Guardian permission tests", t, func() { + orgRoleScenario("Given user has admin org role", m.ROLE_ADMIN, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeTrue) + So(canEdit, ShouldBeTrue) + So(canSave, ShouldBeTrue) + So(canView, ShouldBeTrue) + + Convey("When trying to update permissions", func() { + Convey("With duplicate user/role permissions should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_VIEW}, + {OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianDuplicatePermission) + }) + + Convey("With duplicate team/role permissions should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 1, TeamId: 1, Permission: m.PERMISSION_VIEW}, + {OrgId: 1, DashboardId: 1, TeamId: 1, Permission: m.PERMISSION_ADMIN}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianDuplicatePermission) + }) + + Convey("With duplicate everyone/role permissions should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 1, Permission: m.PERMISSION_VIEW}, + {OrgId: 1, DashboardId: 1, Permission: m.PERMISSION_ADMIN}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianDuplicatePermission) + }) + }) + + Convey("Given parent folder has user admin permission", func() { + existingPermissions := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: 2, UserId: 1, Permission: m.PERMISSION_ADMIN}, + } + + bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { + query.Result = existingPermissions + return nil + }) + + Convey("When trying to update dashboard permissions with admin user permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_ADMIN}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + + Convey("When trying to update dashboard permissions with edit user permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_EDIT}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + + Convey("When trying to update dashboard permissions with view user permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_VIEW}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + }) + + Convey("Given parent folder has user edit permission", func() { + existingPermissions := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: 2, UserId: 1, Permission: m.PERMISSION_EDIT}, + } + + bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { + query.Result = existingPermissions + return nil + }) + + Convey("When trying to update dashboard permissions with admin user permission should be allowed", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_ADMIN}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeTrue) + }) + + Convey("When trying to update dashboard permissions with edit user permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_EDIT}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + + Convey("When trying to update dashboard permissions with view user permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_VIEW}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + }) + + Convey("Given parent folder has user view permission", func() { + existingPermissions := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: 2, UserId: 1, Permission: m.PERMISSION_VIEW}, + } + + bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { + query.Result = existingPermissions + return nil + }) + + Convey("When trying to update dashboard permissions with admin user permission should be allowed", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_ADMIN}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeTrue) + }) + + Convey("When trying to update dashboard permissions with edit user permission should be allowed", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_EDIT}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeTrue) + }) + + Convey("When trying to update dashboard permissions with view user permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_VIEW}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + }) + + Convey("Given parent folder has team admin permission", func() { + existingPermissions := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: 2, TeamId: 1, Permission: m.PERMISSION_ADMIN}, + } + + bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { + query.Result = existingPermissions + return nil + }) + + Convey("When trying to update dashboard permissions with admin team permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_ADMIN}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + + Convey("When trying to update dashboard permissions with edit team permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_EDIT}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + + Convey("When trying to update dashboard permissions with view team permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_VIEW}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + }) + + Convey("Given parent folder has team edit permission", func() { + existingPermissions := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: 2, TeamId: 1, Permission: m.PERMISSION_EDIT}, + } + + bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { + query.Result = existingPermissions + return nil + }) + + Convey("When trying to update dashboard permissions with admin team permission should be allowed", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_ADMIN}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeTrue) + }) + + Convey("When trying to update dashboard permissions with edit team permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_EDIT}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + + Convey("When trying to update dashboard permissions with view team permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_VIEW}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + }) + + Convey("Given parent folder has team view permission", func() { + existingPermissions := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: 2, TeamId: 1, Permission: m.PERMISSION_VIEW}, + } + + bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { + query.Result = existingPermissions + return nil + }) + + Convey("When trying to update dashboard permissions with admin team permission should be allowed", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_ADMIN}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeTrue) + }) + + Convey("When trying to update dashboard permissions with edit team permission should be allowed", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_EDIT}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeTrue) + }) + + Convey("When trying to update dashboard permissions with view team permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_VIEW}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + }) + + Convey("Given parent folder has editor role with edit permission", func() { + r := m.ROLE_EDITOR + existingPermissions := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: 2, Role: &r, Permission: m.PERMISSION_EDIT}, + } + + bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { + query.Result = existingPermissions + return nil + }) + + Convey("When trying to update dashboard permissions with everyone with editor role can admin permission should be allowed", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, Role: &r, Permission: m.PERMISSION_ADMIN}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeTrue) + }) + + Convey("When trying to update dashboard permissions with everyone with editor role can edit permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, Role: &r, Permission: m.PERMISSION_EDIT}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + + Convey("When trying to update dashboard permissions with everyone with editor role can view permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, Role: &r, Permission: m.PERMISSION_VIEW}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + }) + + Convey("Given parent folder has editor role with view permission", func() { + r := m.ROLE_EDITOR + existingPermissions := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: 2, Role: &r, Permission: m.PERMISSION_VIEW}, + } + + bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { + query.Result = existingPermissions + return nil + }) + + Convey("When trying to update dashboard permissions with everyone with viewer role can admin permission should be allowed", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, Role: &r, Permission: m.PERMISSION_ADMIN}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeTrue) + }) + + Convey("When trying to update dashboard permissions with everyone with viewer role can edit permission should be allowed", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, Role: &r, Permission: m.PERMISSION_EDIT}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeTrue) + }) + + Convey("When trying to update dashboard permissions with everyone with viewer role can view permission should return error", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 3, Role: &r, Permission: m.PERMISSION_VIEW}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + }) + }) + }) + + orgRoleScenario("Given user has editor org role", m.ROLE_EDITOR, func(sc *scenarioContext) { + everyoneWithRoleScenario(m.ROLE_EDITOR, m.PERMISSION_ADMIN, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeTrue) + So(canEdit, ShouldBeTrue) + So(canSave, ShouldBeTrue) + So(canView, ShouldBeTrue) + }) + + everyoneWithRoleScenario(m.ROLE_EDITOR, m.PERMISSION_EDIT, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeTrue) + So(canSave, ShouldBeTrue) + So(canView, ShouldBeTrue) + }) + + everyoneWithRoleScenario(m.ROLE_EDITOR, m.PERMISSION_VIEW, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeFalse) + So(canSave, ShouldBeFalse) + So(canView, ShouldBeTrue) + }) + + everyoneWithRoleScenario(m.ROLE_VIEWER, m.PERMISSION_ADMIN, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeFalse) + So(canSave, ShouldBeFalse) + So(canView, ShouldBeFalse) + }) + + everyoneWithRoleScenario(m.ROLE_VIEWER, m.PERMISSION_EDIT, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeFalse) + So(canSave, ShouldBeFalse) + So(canView, ShouldBeFalse) + }) + + everyoneWithRoleScenario(m.ROLE_VIEWER, m.PERMISSION_VIEW, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeFalse) + So(canSave, ShouldBeFalse) + So(canView, ShouldBeFalse) + }) + + userWithPermissionScenario(m.PERMISSION_ADMIN, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeTrue) + So(canEdit, ShouldBeTrue) + So(canSave, ShouldBeTrue) + So(canView, ShouldBeTrue) + }) + + userWithPermissionScenario(m.PERMISSION_EDIT, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeTrue) + So(canSave, ShouldBeTrue) + So(canView, ShouldBeTrue) + }) + + userWithPermissionScenario(m.PERMISSION_VIEW, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeFalse) + So(canSave, ShouldBeFalse) + So(canView, ShouldBeTrue) + }) + + teamWithPermissionScenario(m.PERMISSION_ADMIN, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeTrue) + So(canEdit, ShouldBeTrue) + So(canSave, ShouldBeTrue) + So(canView, ShouldBeTrue) + }) + + teamWithPermissionScenario(m.PERMISSION_EDIT, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeTrue) + So(canSave, ShouldBeTrue) + So(canView, ShouldBeTrue) + }) + + teamWithPermissionScenario(m.PERMISSION_VIEW, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeFalse) + So(canSave, ShouldBeFalse) + So(canView, ShouldBeTrue) + }) + + Convey("When trying to update permissions should return false", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_VIEW}, + {OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeFalse) + }) + }) + + orgRoleScenario("Given user has viewer org role", m.ROLE_VIEWER, func(sc *scenarioContext) { + everyoneWithRoleScenario(m.ROLE_EDITOR, m.PERMISSION_ADMIN, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeFalse) + So(canSave, ShouldBeFalse) + So(canView, ShouldBeFalse) + }) + + everyoneWithRoleScenario(m.ROLE_EDITOR, m.PERMISSION_EDIT, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeFalse) + So(canSave, ShouldBeFalse) + So(canView, ShouldBeFalse) + }) + + everyoneWithRoleScenario(m.ROLE_EDITOR, m.PERMISSION_VIEW, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeFalse) + So(canSave, ShouldBeFalse) + So(canView, ShouldBeFalse) + }) + + everyoneWithRoleScenario(m.ROLE_VIEWER, m.PERMISSION_ADMIN, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeTrue) + So(canEdit, ShouldBeTrue) + So(canSave, ShouldBeTrue) + So(canView, ShouldBeTrue) + }) + + everyoneWithRoleScenario(m.ROLE_VIEWER, m.PERMISSION_EDIT, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeTrue) + So(canSave, ShouldBeTrue) + So(canView, ShouldBeTrue) + }) + + everyoneWithRoleScenario(m.ROLE_VIEWER, m.PERMISSION_VIEW, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeFalse) + So(canSave, ShouldBeFalse) + So(canView, ShouldBeTrue) + }) + + userWithPermissionScenario(m.PERMISSION_ADMIN, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeTrue) + So(canEdit, ShouldBeTrue) + So(canSave, ShouldBeTrue) + So(canView, ShouldBeTrue) + }) + + userWithPermissionScenario(m.PERMISSION_EDIT, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeTrue) + So(canSave, ShouldBeTrue) + So(canView, ShouldBeTrue) + }) + + userWithPermissionScenario(m.PERMISSION_VIEW, sc, func(sc *scenarioContext) { + canAdmin, _ := sc.g.CanAdmin() + canEdit, _ := sc.g.CanEdit() + canSave, _ := sc.g.CanSave() + canView, _ := sc.g.CanView() + So(canAdmin, ShouldBeFalse) + So(canEdit, ShouldBeFalse) + So(canSave, ShouldBeFalse) + So(canView, ShouldBeTrue) + }) + + Convey("When trying to update permissions should return false", func() { + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_VIEW}, + {OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeFalse) + }) + }) + }) +} + +type scenarioContext struct { + g DashboardGuardian +} + +type scenarioFunc func(c *scenarioContext) + +func orgRoleScenario(desc string, role m.RoleType, fn scenarioFunc) { + user := &m.SignedInUser{ + UserId: 1, + OrgId: 1, + OrgRole: role, + } + guard := New(1, 1, user) + sc := &scenarioContext{ + g: guard, + } + + Convey(desc, func() { + fn(sc) + }) +} + +func permissionScenario(desc string, sc *scenarioContext, permissions []*m.DashboardAclInfoDTO, fn scenarioFunc) { + bus.ClearBusHandlers() + + bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { + query.Result = permissions + return nil + }) + + teams := []*m.Team{} + + for _, p := range permissions { + if p.TeamId > 0 { + teams = append(teams, &m.Team{Id: p.TeamId}) + } + } + + bus.AddHandler("test", func(query *m.GetTeamsByUserQuery) error { + query.Result = teams + return nil + }) + + Convey(desc, func() { + fn(sc) + }) +} + +func userWithPermissionScenario(permission m.PermissionType, sc *scenarioContext, fn scenarioFunc) { + p := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: 1, UserId: 1, Permission: permission}, + } + permissionScenario(fmt.Sprintf("and user has permission to %s item", permission), sc, p, fn) +} + +func teamWithPermissionScenario(permission m.PermissionType, sc *scenarioContext, fn scenarioFunc) { + p := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: 1, TeamId: 1, Permission: permission}, + } + permissionScenario(fmt.Sprintf("and team has permission to %s item", permission), sc, p, fn) +} + +func everyoneWithRoleScenario(role m.RoleType, permission m.PermissionType, sc *scenarioContext, fn scenarioFunc) { + p := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: 1, UserId: -1, Role: &role, Permission: permission}, + } + permissionScenario(fmt.Sprintf("and everyone with %s role can %s item", role, permission), sc, p, fn) +} From 3c14cecd506feb99711efaac0580e0ab6c134c26 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Mon, 26 Feb 2018 20:14:21 +0100 Subject: [PATCH 2/6] folders: handle new guardian error responses and add tests --- pkg/api/folder_permission.go | 17 +++-- pkg/api/folder_permission_test.go | 119 +++++++++++++++++++++++++++++- 2 files changed, 129 insertions(+), 7 deletions(-) diff --git a/pkg/api/folder_permission.go b/pkg/api/folder_permission.go index 7453552d092..c4c762dc3cd 100644 --- a/pkg/api/folder_permission.go +++ b/pkg/api/folder_permission.go @@ -19,13 +19,13 @@ func GetFolderPermissionList(c *middleware.Context) Response { return toFolderError(err) } - guardian := guardian.New(folder.Id, c.OrgId, c.SignedInUser) + g := guardian.New(folder.Id, c.OrgId, c.SignedInUser) - if canAdmin, err := guardian.CanAdmin(); err != nil || !canAdmin { + if canAdmin, err := g.CanAdmin(); err != nil || !canAdmin { return toFolderError(m.ErrFolderAccessDenied) } - acl, err := guardian.GetAcl() + acl, err := g.GetAcl() if err != nil { return ApiError(500, "Failed to get folder permissions", err) } @@ -50,8 +50,8 @@ func UpdateFolderPermissions(c *middleware.Context, apiCmd dtos.UpdateDashboardA return toFolderError(err) } - guardian := guardian.New(folder.Id, c.OrgId, c.SignedInUser) - canAdmin, err := guardian.CanAdmin() + g := guardian.New(folder.Id, c.OrgId, c.SignedInUser) + canAdmin, err := g.CanAdmin() if err != nil { return toFolderError(err) } @@ -76,8 +76,13 @@ func UpdateFolderPermissions(c *middleware.Context, apiCmd dtos.UpdateDashboardA }) } - if okToUpdate, err := guardian.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, cmd.Items); err != nil || !okToUpdate { + if okToUpdate, err := g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, cmd.Items); err != nil || !okToUpdate { if err != nil { + if err == guardian.ErrGuardianDuplicatePermission || + err == guardian.ErrGuardianOverrideLowerPresedence { + return ApiError(400, err.Error(), err) + } + return ApiError(500, "Error while checking folder permissions", err) } diff --git a/pkg/api/folder_permission_test.go b/pkg/api/folder_permission_test.go index bbae5390b80..bc35031ce52 100644 --- a/pkg/api/folder_permission_test.go +++ b/pkg/api/folder_permission_test.go @@ -5,6 +5,7 @@ import ( "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/middleware" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/dashboards" @@ -15,6 +16,35 @@ import ( func TestFolderPermissionApiEndpoint(t *testing.T) { Convey("Folder permissions test", t, func() { + Convey("Given folder not exists", func() { + mock := &fakeFolderService{ + GetFolderByUidError: m.ErrFolderNotFound, + } + + origNewFolderService := dashboards.NewFolderService + mockFolderService(mock) + + loggedInUserScenarioWithRole("When calling GET on", "GET", "/api/folders/uid/permissions", "/api/folders/:uid/permissions", m.ROLE_EDITOR, func(sc *scenarioContext) { + callGetFolderPermissions(sc) + So(sc.resp.Code, ShouldEqual, 404) + }) + + cmd := dtos.UpdateDashboardAclCommand{ + Items: []dtos.DashboardAclUpdateItem{ + {UserId: 1000, Permission: m.PERMISSION_ADMIN}, + }, + } + + updateFolderPermissionScenario("When calling POST on", "/api/folders/uid/permissions", "/api/folders/:uid/permissions", cmd, func(sc *scenarioContext) { + callUpdateFolderPermissions(sc) + So(sc.resp.Code, ShouldEqual, 404) + }) + + Reset(func() { + dashboards.NewFolderService = origNewFolderService + }) + }) + Convey("Given user has no admin permissions", func() { origNewGuardian := guardian.New guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanAdminValue: false}) @@ -54,7 +84,17 @@ func TestFolderPermissionApiEndpoint(t *testing.T) { Convey("Given user has admin permissions and permissions to update", func() { origNewGuardian := guardian.New - guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanAdminValue: true, CheckPermissionBeforeUpdateValue: true}) + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{ + CanAdminValue: true, + CheckPermissionBeforeUpdateValue: true, + GetAclValue: []*m.DashboardAclInfoDTO{ + {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}, + }, + }) mock := &fakeFolderService{ GetFolderByUidResult: &m.Folder{ @@ -70,6 +110,11 @@ func TestFolderPermissionApiEndpoint(t *testing.T) { loggedInUserScenarioWithRole("When calling GET on", "GET", "/api/folders/uid/permissions", "/api/folders/:uid/permissions", m.ROLE_ADMIN, func(sc *scenarioContext) { callGetFolderPermissions(sc) So(sc.resp.Code, ShouldEqual, 200) + respJSON, err := simplejson.NewJson(sc.resp.Body.Bytes()) + So(err, ShouldBeNil) + So(len(respJSON.MustArray()), ShouldEqual, 5) + So(respJSON.GetIndex(0).Get("userId").MustInt(), ShouldEqual, 2) + So(respJSON.GetIndex(0).Get("permission").MustInt(), ShouldEqual, m.PERMISSION_VIEW) }) cmd := dtos.UpdateDashboardAclCommand{ @@ -88,6 +133,78 @@ func TestFolderPermissionApiEndpoint(t *testing.T) { dashboards.NewFolderService = origNewFolderService }) }) + + Convey("When trying to update permissions with duplicate permissions", func() { + origNewGuardian := guardian.New + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{ + CanAdminValue: true, + CheckPermissionBeforeUpdateValue: false, + CheckPermissionBeforeUpdateError: guardian.ErrGuardianDuplicatePermission, + }) + + mock := &fakeFolderService{ + GetFolderByUidResult: &m.Folder{ + Id: 1, + Uid: "uid", + Title: "Folder", + }, + } + + origNewFolderService := dashboards.NewFolderService + mockFolderService(mock) + + cmd := dtos.UpdateDashboardAclCommand{ + Items: []dtos.DashboardAclUpdateItem{ + {UserId: 1000, Permission: m.PERMISSION_ADMIN}, + }, + } + + updateFolderPermissionScenario("When calling POST on", "/api/folders/uid/permissions", "/api/folders/:uid/permissions", cmd, func(sc *scenarioContext) { + callUpdateFolderPermissions(sc) + So(sc.resp.Code, ShouldEqual, 400) + }) + + Reset(func() { + guardian.New = origNewGuardian + dashboards.NewFolderService = origNewFolderService + }) + }) + + Convey("When trying to override inherited permissions with lower presedence", func() { + origNewGuardian := guardian.New + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{ + CanAdminValue: true, + CheckPermissionBeforeUpdateValue: false, + CheckPermissionBeforeUpdateError: guardian.ErrGuardianOverrideLowerPresedence}, + ) + + mock := &fakeFolderService{ + GetFolderByUidResult: &m.Folder{ + Id: 1, + Uid: "uid", + Title: "Folder", + }, + } + + origNewFolderService := dashboards.NewFolderService + mockFolderService(mock) + + cmd := dtos.UpdateDashboardAclCommand{ + Items: []dtos.DashboardAclUpdateItem{ + {UserId: 1000, Permission: m.PERMISSION_ADMIN}, + }, + } + + updateFolderPermissionScenario("When calling POST on", "/api/folders/uid/permissions", "/api/folders/:uid/permissions", cmd, func(sc *scenarioContext) { + callUpdateFolderPermissions(sc) + So(sc.resp.Code, ShouldEqual, 400) + }) + + Reset(func() { + guardian.New = origNewGuardian + dashboards.NewFolderService = origNewFolderService + }) + }) }) } From 03f8eff8800176759fcb971cfadcb55b6746534b Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Mon, 26 Feb 2018 20:15:57 +0100 Subject: [PATCH 3/6] dashboards: handle new guardian error responses and update tests Using a mocked guardian instead and relies on the actual guardian tests for verifying permission handling correctness --- pkg/api/dashboard_permission.go | 17 +- pkg/api/dashboard_permission_test.go | 290 +++++++++++++-------------- 2 files changed, 154 insertions(+), 153 deletions(-) diff --git a/pkg/api/dashboard_permission.go b/pkg/api/dashboard_permission.go index 351b81885d6..dafa5777803 100644 --- a/pkg/api/dashboard_permission.go +++ b/pkg/api/dashboard_permission.go @@ -18,13 +18,13 @@ func GetDashboardPermissionList(c *middleware.Context) Response { return rsp } - guardian := guardian.New(dashId, c.OrgId, c.SignedInUser) + g := guardian.New(dashId, c.OrgId, c.SignedInUser) - if canAdmin, err := guardian.CanAdmin(); err != nil || !canAdmin { + if canAdmin, err := g.CanAdmin(); err != nil || !canAdmin { return dashboardGuardianResponse(err) } - acl, err := guardian.GetAcl() + acl, err := g.GetAcl() if err != nil { return ApiError(500, "Failed to get dashboard permissions", err) } @@ -46,8 +46,8 @@ func UpdateDashboardPermissions(c *middleware.Context, apiCmd dtos.UpdateDashboa return rsp } - guardian := guardian.New(dashId, c.OrgId, c.SignedInUser) - if canAdmin, err := guardian.CanAdmin(); err != nil || !canAdmin { + g := guardian.New(dashId, c.OrgId, c.SignedInUser) + if canAdmin, err := g.CanAdmin(); err != nil || !canAdmin { return dashboardGuardianResponse(err) } @@ -67,8 +67,13 @@ func UpdateDashboardPermissions(c *middleware.Context, apiCmd dtos.UpdateDashboa }) } - if okToUpdate, err := guardian.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, cmd.Items); err != nil || !okToUpdate { + if okToUpdate, err := g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, cmd.Items); err != nil || !okToUpdate { if err != nil { + if err == guardian.ErrGuardianDuplicatePermission || + err == guardian.ErrGuardianOverrideLowerPresedence { + return ApiError(400, err.Error(), err) + } + return ApiError(500, "Error while checking dashboard permissions", err) } diff --git a/pkg/api/dashboard_permission_test.go b/pkg/api/dashboard_permission_test.go index fd399cfb096..0bb9908df6d 100644 --- a/pkg/api/dashboard_permission_test.go +++ b/pkg/api/dashboard_permission_test.go @@ -8,183 +8,180 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/middleware" m "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/guardian" . "github.com/smartystreets/goconvey/convey" ) func TestDashboardPermissionApiEndpoint(t *testing.T) { - Convey("Given a dashboard with permissions", t, func() { - mockResult := []*m.DashboardAclInfoDTO{ - {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) - - getDashboardQueryResult := m.NewDashboard("Dash") - var getDashboardNotFoundError error - - bus.AddHandler("test", func(query *m.GetDashboardQuery) error { - query.Result = getDashboardQueryResult - return getDashboardNotFoundError - }) - - bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { - query.Result = dtoRes - return nil - }) - - bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { - query.Result = mockResult - return nil - }) - - teamResp := []*m.Team{} - bus.AddHandler("test", func(query *m.GetTeamsByUserQuery) error { - query.Result = teamResp - return nil - }) - - // This tests four scenarios: - // 1. user is an org admin - // 2. user is an org editor AND has been granted admin permission for the dashboard - // 3. user is an org viewer AND has been granted edit permission for the dashboard - // 4. user is an org editor AND has no permissions for the dashboard - - Convey("When user is org admin", func() { - loggedInUserScenarioWithRole("When calling GET on", "GET", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:dashboardsId/permissions", m.ROLE_ADMIN, func(sc *scenarioContext) { - Convey("Should be able to access ACL", func() { - sc.handlerFunc = GetDashboardPermissionList - sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() - - So(sc.resp.Code, ShouldEqual, 200) - - respJSON, err := simplejson.NewJson(sc.resp.Body.Bytes()) - So(err, ShouldBeNil) - So(len(respJSON.MustArray()), ShouldEqual, 5) - So(respJSON.GetIndex(0).Get("userId").MustInt(), ShouldEqual, 2) - So(respJSON.GetIndex(0).Get("permission").MustInt(), ShouldEqual, m.PERMISSION_VIEW) - }) + Convey("Dashboard permissions test", t, func() { + Convey("Given dashboard not exists", func() { + bus.AddHandler("test", func(query *m.GetDashboardQuery) error { + return m.ErrDashboardNotFound }) - loggedInUserScenarioWithRole("When calling GET on", "GET", "/api/dashboards/id/2/permissions", "/api/dashboards/id/:dashboardId/permissions", m.ROLE_ADMIN, func(sc *scenarioContext) { - getDashboardNotFoundError = m.ErrDashboardNotFound - sc.handlerFunc = GetDashboardPermissionList - sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() - - Convey("Should not be able to access ACL", func() { - So(sc.resp.Code, ShouldEqual, 404) - }) + loggedInUserScenarioWithRole("When calling GET on", "GET", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:id/permissions", m.ROLE_EDITOR, func(sc *scenarioContext) { + callGetDashboardPermissions(sc) + So(sc.resp.Code, ShouldEqual, 404) }) - Convey("Should not be able to update permissions for non-existing dashboard", func() { - cmd := dtos.UpdateDashboardAclCommand{ - Items: []dtos.DashboardAclUpdateItem{ - {UserId: 1000, Permission: m.PERMISSION_ADMIN}, - }, - } + cmd := dtos.UpdateDashboardAclCommand{ + Items: []dtos.DashboardAclUpdateItem{ + {UserId: 1000, Permission: m.PERMISSION_ADMIN}, + }, + } - postAclScenario("When calling POST on", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:dashboardId/permissions", m.ROLE_ADMIN, cmd, func(sc *scenarioContext) { - getDashboardNotFoundError = m.ErrDashboardNotFound - CallPostAcl(sc) - So(sc.resp.Code, ShouldEqual, 404) - }) + updateDashboardPermissionScenario("When calling POST on", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:id/permissions", cmd, func(sc *scenarioContext) { + callUpdateDashboardPermissions(sc) + 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/permissions", "/api/dashboards/id/:dashboardId/permissions", m.ROLE_EDITOR, func(sc *scenarioContext) { - mockResult = append(mockResult, &m.DashboardAclInfoDTO{OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}) + Convey("Given user has no admin permissions", func() { + origNewGuardian := guardian.New + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanAdminValue: false}) - Convey("Should be able to access ACL", func() { - sc.handlerFunc = GetDashboardPermissionList - sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() - - So(sc.resp.Code, ShouldEqual, 200) - }) + getDashboardQueryResult := m.NewDashboard("Dash") + bus.AddHandler("test", func(query *m.GetDashboardQuery) error { + query.Result = getDashboardQueryResult + return nil }) - Convey("Should not be able to downgrade their own Admin permission", func() { - cmd := dtos.UpdateDashboardAclCommand{ - Items: []dtos.DashboardAclUpdateItem{ - {UserId: TestUserID, Permission: m.PERMISSION_EDIT}, - }, - } - - postAclScenario("When calling POST on", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:dashboardId/permissions", m.ROLE_EDITOR, cmd, func(sc *scenarioContext) { - mockResult = append(mockResult, &m.DashboardAclInfoDTO{OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}) - - CallPostAcl(sc) - So(sc.resp.Code, ShouldEqual, 403) - }) + loggedInUserScenarioWithRole("When calling GET on", "GET", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:id/permissions", m.ROLE_EDITOR, func(sc *scenarioContext) { + callGetDashboardPermissions(sc) + So(sc.resp.Code, ShouldEqual, 403) }) - Convey("Should be able to update permissions", func() { - cmd := dtos.UpdateDashboardAclCommand{ - Items: []dtos.DashboardAclUpdateItem{ - {UserId: TestUserID, Permission: m.PERMISSION_ADMIN}, - {UserId: 2, Permission: m.PERMISSION_EDIT}, - }, - } + cmd := dtos.UpdateDashboardAclCommand{ + Items: []dtos.DashboardAclUpdateItem{ + {UserId: 1000, Permission: m.PERMISSION_ADMIN}, + }, + } - postAclScenario("When calling POST on", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:dashboardId/permissions", m.ROLE_EDITOR, cmd, func(sc *scenarioContext) { - mockResult = append(mockResult, &m.DashboardAclInfoDTO{OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}) - - CallPostAcl(sc) - So(sc.resp.Code, ShouldEqual, 200) - }) + updateDashboardPermissionScenario("When calling POST on", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:id/permissions", cmd, func(sc *scenarioContext) { + callUpdateDashboardPermissions(sc) + So(sc.resp.Code, ShouldEqual, 403) }) - }) - - Convey("When user is org viewer and has edit permission in the ACL", func() { - loggedInUserScenarioWithRole("When calling GET on", "GET", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:dashboardId/permissions", m.ROLE_VIEWER, func(sc *scenarioContext) { - 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() { - sc.handlerFunc = GetDashboardPermissionList - sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() - - So(sc.resp.Code, ShouldEqual, 403) - }) + Reset(func() { + guardian.New = origNewGuardian }) }) - Convey("When user is org editor and not in the ACL", func() { - loggedInUserScenarioWithRole("When calling GET on", "GET", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:dashboardsId/permissions", m.ROLE_EDITOR, func(sc *scenarioContext) { + Convey("Given user has admin permissions and permissions to update", func() { + origNewGuardian := guardian.New + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{ + CanAdminValue: true, + CheckPermissionBeforeUpdateValue: true, + GetAclValue: []*m.DashboardAclInfoDTO{ + {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}, + }, + }) - Convey("Should not be able to access ACL", func() { - sc.handlerFunc = GetDashboardPermissionList - sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() + getDashboardQueryResult := m.NewDashboard("Dash") + bus.AddHandler("test", func(query *m.GetDashboardQuery) error { + query.Result = getDashboardQueryResult + return nil + }) - So(sc.resp.Code, ShouldEqual, 403) - }) + loggedInUserScenarioWithRole("When calling GET on", "GET", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:id/permissions", m.ROLE_ADMIN, func(sc *scenarioContext) { + callGetDashboardPermissions(sc) + So(sc.resp.Code, ShouldEqual, 200) + respJSON, err := simplejson.NewJson(sc.resp.Body.Bytes()) + So(err, ShouldBeNil) + So(len(respJSON.MustArray()), ShouldEqual, 5) + So(respJSON.GetIndex(0).Get("userId").MustInt(), ShouldEqual, 2) + So(respJSON.GetIndex(0).Get("permission").MustInt(), ShouldEqual, m.PERMISSION_VIEW) + }) + + cmd := dtos.UpdateDashboardAclCommand{ + Items: []dtos.DashboardAclUpdateItem{ + {UserId: 1000, Permission: m.PERMISSION_ADMIN}, + }, + } + + updateDashboardPermissionScenario("When calling POST on", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:id/permissions", cmd, func(sc *scenarioContext) { + callUpdateDashboardPermissions(sc) + So(sc.resp.Code, ShouldEqual, 200) + }) + + Reset(func() { + guardian.New = origNewGuardian + }) + }) + + Convey("When trying to update permissions with duplicate permissions", func() { + origNewGuardian := guardian.New + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{ + CanAdminValue: true, + CheckPermissionBeforeUpdateValue: false, + CheckPermissionBeforeUpdateError: guardian.ErrGuardianDuplicatePermission, + }) + + getDashboardQueryResult := m.NewDashboard("Dash") + bus.AddHandler("test", func(query *m.GetDashboardQuery) error { + query.Result = getDashboardQueryResult + return nil + }) + + cmd := dtos.UpdateDashboardAclCommand{ + Items: []dtos.DashboardAclUpdateItem{ + {UserId: 1000, Permission: m.PERMISSION_ADMIN}, + }, + } + + updateDashboardPermissionScenario("When calling POST on", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:id/permissions", cmd, func(sc *scenarioContext) { + callUpdateDashboardPermissions(sc) + So(sc.resp.Code, ShouldEqual, 400) + }) + + Reset(func() { + guardian.New = origNewGuardian + }) + }) + + Convey("When trying to override inherited permissions with lower presedence", func() { + origNewGuardian := guardian.New + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{ + CanAdminValue: true, + CheckPermissionBeforeUpdateValue: false, + CheckPermissionBeforeUpdateError: guardian.ErrGuardianOverrideLowerPresedence}, + ) + + getDashboardQueryResult := m.NewDashboard("Dash") + bus.AddHandler("test", func(query *m.GetDashboardQuery) error { + query.Result = getDashboardQueryResult + return nil + }) + + cmd := dtos.UpdateDashboardAclCommand{ + Items: []dtos.DashboardAclUpdateItem{ + {UserId: 1000, Permission: m.PERMISSION_ADMIN}, + }, + } + + updateDashboardPermissionScenario("When calling POST on", "/api/dashboards/id/1/permissions", "/api/dashboards/id/:id/permissions", cmd, func(sc *scenarioContext) { + callUpdateDashboardPermissions(sc) + So(sc.resp.Code, ShouldEqual, 400) + }) + + Reset(func() { + guardian.New = origNewGuardian }) }) }) } -func transformDashboardAclsToDTOs(acls []*m.DashboardAclInfoDTO) []*m.DashboardAclInfoDTO { - dtos := make([]*m.DashboardAclInfoDTO, 0) - - for _, acl := range acls { - dto := &m.DashboardAclInfoDTO{ - OrgId: acl.OrgId, - DashboardId: acl.DashboardId, - Permission: acl.Permission, - UserId: acl.UserId, - TeamId: acl.TeamId, - } - dtos = append(dtos, dto) - } - - return dtos +func callGetDashboardPermissions(sc *scenarioContext) { + sc.handlerFunc = GetDashboardPermissionList + sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() } -func CallPostAcl(sc *scenarioContext) { +func callUpdateDashboardPermissions(sc *scenarioContext) { bus.AddHandler("test", func(cmd *m.UpdateDashboardAclCommand) error { return nil }) @@ -192,7 +189,7 @@ func CallPostAcl(sc *scenarioContext) { sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() } -func postAclScenario(desc string, url string, routePattern string, role m.RoleType, cmd dtos.UpdateDashboardAclCommand, fn scenarioFunc) { +func updateDashboardPermissionScenario(desc string, url string, routePattern string, cmd dtos.UpdateDashboardAclCommand, fn scenarioFunc) { Convey(desc+" "+url, func() { defer bus.ClearBusHandlers() @@ -200,9 +197,8 @@ func postAclScenario(desc string, url string, routePattern string, role m.RoleTy sc.defaultHandler = wrap(func(c *middleware.Context) Response { sc.context = c - sc.context.UserId = TestUserID sc.context.OrgId = TestOrgID - sc.context.OrgRole = role + sc.context.UserId = TestUserID return UpdateDashboardPermissions(c, cmd) }) From f76b98d252b70b10a716c9e6329d8a9917006280 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Tue, 27 Feb 2018 16:03:11 +0100 Subject: [PATCH 4/6] dashboards: change dashboard/folder permission error messages --- pkg/api/dashboard_permission.go | 4 +-- pkg/api/dashboard_permission_test.go | 4 +-- pkg/api/folder_permission.go | 4 +-- pkg/api/folder_permission_test.go | 4 +-- pkg/services/guardian/guardian.go | 8 +++--- pkg/services/guardian/guardian_test.go | 36 +++++++++++++------------- 6 files changed, 30 insertions(+), 30 deletions(-) diff --git a/pkg/api/dashboard_permission.go b/pkg/api/dashboard_permission.go index dafa5777803..419825644c8 100644 --- a/pkg/api/dashboard_permission.go +++ b/pkg/api/dashboard_permission.go @@ -69,8 +69,8 @@ func UpdateDashboardPermissions(c *middleware.Context, apiCmd dtos.UpdateDashboa if okToUpdate, err := g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, cmd.Items); err != nil || !okToUpdate { if err != nil { - if err == guardian.ErrGuardianDuplicatePermission || - err == guardian.ErrGuardianOverrideLowerPresedence { + if err == guardian.ErrGuardianPermissionExists || + err == guardian.ErrGuardianOverride { return ApiError(400, err.Error(), err) } diff --git a/pkg/api/dashboard_permission_test.go b/pkg/api/dashboard_permission_test.go index 0bb9908df6d..03231338268 100644 --- a/pkg/api/dashboard_permission_test.go +++ b/pkg/api/dashboard_permission_test.go @@ -119,7 +119,7 @@ func TestDashboardPermissionApiEndpoint(t *testing.T) { guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{ CanAdminValue: true, CheckPermissionBeforeUpdateValue: false, - CheckPermissionBeforeUpdateError: guardian.ErrGuardianDuplicatePermission, + CheckPermissionBeforeUpdateError: guardian.ErrGuardianPermissionExists, }) getDashboardQueryResult := m.NewDashboard("Dash") @@ -149,7 +149,7 @@ func TestDashboardPermissionApiEndpoint(t *testing.T) { guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{ CanAdminValue: true, CheckPermissionBeforeUpdateValue: false, - CheckPermissionBeforeUpdateError: guardian.ErrGuardianOverrideLowerPresedence}, + CheckPermissionBeforeUpdateError: guardian.ErrGuardianOverride}, ) getDashboardQueryResult := m.NewDashboard("Dash") diff --git a/pkg/api/folder_permission.go b/pkg/api/folder_permission.go index c4c762dc3cd..7c8aba87337 100644 --- a/pkg/api/folder_permission.go +++ b/pkg/api/folder_permission.go @@ -78,8 +78,8 @@ func UpdateFolderPermissions(c *middleware.Context, apiCmd dtos.UpdateDashboardA if okToUpdate, err := g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, cmd.Items); err != nil || !okToUpdate { if err != nil { - if err == guardian.ErrGuardianDuplicatePermission || - err == guardian.ErrGuardianOverrideLowerPresedence { + if err == guardian.ErrGuardianPermissionExists || + err == guardian.ErrGuardianOverride { return ApiError(400, err.Error(), err) } diff --git a/pkg/api/folder_permission_test.go b/pkg/api/folder_permission_test.go index bc35031ce52..552577963b4 100644 --- a/pkg/api/folder_permission_test.go +++ b/pkg/api/folder_permission_test.go @@ -139,7 +139,7 @@ func TestFolderPermissionApiEndpoint(t *testing.T) { guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{ CanAdminValue: true, CheckPermissionBeforeUpdateValue: false, - CheckPermissionBeforeUpdateError: guardian.ErrGuardianDuplicatePermission, + CheckPermissionBeforeUpdateError: guardian.ErrGuardianPermissionExists, }) mock := &fakeFolderService{ @@ -175,7 +175,7 @@ func TestFolderPermissionApiEndpoint(t *testing.T) { guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{ CanAdminValue: true, CheckPermissionBeforeUpdateValue: false, - CheckPermissionBeforeUpdateError: guardian.ErrGuardianOverrideLowerPresedence}, + CheckPermissionBeforeUpdateError: guardian.ErrGuardianOverride}, ) mock := &fakeFolderService{ diff --git a/pkg/services/guardian/guardian.go b/pkg/services/guardian/guardian.go index 972e87de39b..54b5386f40d 100644 --- a/pkg/services/guardian/guardian.go +++ b/pkg/services/guardian/guardian.go @@ -10,8 +10,8 @@ import ( ) var ( - ErrGuardianDuplicatePermission = errors.New("You cannot add multiple permissions for a user, team or role") - ErrGuardianOverrideLowerPresedence = errors.New("You cannot override a permission with a lower presedence permission") + ErrGuardianPermissionExists = errors.New("This permission already exists") + ErrGuardianOverride = errors.New("You can only override a permission to be higher") ) // DashboardGuardian to be used for guard against operations without access on dashboard and acl @@ -133,7 +133,7 @@ func (g *dashboardGuardianImpl) CheckPermissionBeforeUpdate(permission m.Permiss if (a.UserId <= 0 && a.TeamId <= 0 && a.UserId == p.UserId && a.TeamId == p.TeamId && a.Role == p.Role) || (a.UserId > 0 && a.UserId == p.UserId) || (a.TeamId > 0 && a.TeamId == p.TeamId) { - return false, ErrGuardianDuplicatePermission + return false, ErrGuardianPermissionExists } } @@ -154,7 +154,7 @@ func (g *dashboardGuardianImpl) CheckPermissionBeforeUpdate(permission m.Permiss if (a.UserId <= 0 && a.TeamId <= 0 && a.UserId == existingPerm.UserId && a.TeamId == existingPerm.TeamId && *a.Role == *existingPerm.Role && a.Permission <= existingPerm.Permission) || (a.UserId > 0 && a.UserId == existingPerm.UserId && a.Permission <= existingPerm.Permission) || (a.TeamId > 0 && a.TeamId == existingPerm.TeamId && a.Permission <= existingPerm.Permission) { - return false, ErrGuardianOverrideLowerPresedence + return false, ErrGuardianOverride } } } diff --git a/pkg/services/guardian/guardian_test.go b/pkg/services/guardian/guardian_test.go index d420d1add97..0da14a51c5c 100644 --- a/pkg/services/guardian/guardian_test.go +++ b/pkg/services/guardian/guardian_test.go @@ -29,7 +29,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianDuplicatePermission) + So(err, ShouldEqual, ErrGuardianPermissionExists) }) Convey("With duplicate team/role permissions should return error", func() { @@ -38,7 +38,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 1, TeamId: 1, Permission: m.PERMISSION_ADMIN}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianDuplicatePermission) + So(err, ShouldEqual, ErrGuardianPermissionExists) }) Convey("With duplicate everyone/role permissions should return error", func() { @@ -47,7 +47,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 1, Permission: m.PERMISSION_ADMIN}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianDuplicatePermission) + So(err, ShouldEqual, ErrGuardianPermissionExists) }) }) @@ -66,7 +66,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_ADMIN}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) Convey("When trying to update dashboard permissions with edit user permission should return error", func() { @@ -74,7 +74,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_EDIT}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) Convey("When trying to update dashboard permissions with view user permission should return error", func() { @@ -82,7 +82,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_VIEW}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) }) @@ -109,7 +109,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_EDIT}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) Convey("When trying to update dashboard permissions with view user permission should return error", func() { @@ -117,7 +117,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_VIEW}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) }) @@ -152,7 +152,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, UserId: 1, Permission: m.PERMISSION_VIEW}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) }) @@ -171,7 +171,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_ADMIN}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) Convey("When trying to update dashboard permissions with edit team permission should return error", func() { @@ -179,7 +179,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_EDIT}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) Convey("When trying to update dashboard permissions with view team permission should return error", func() { @@ -187,7 +187,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_VIEW}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) }) @@ -214,7 +214,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_EDIT}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) Convey("When trying to update dashboard permissions with view team permission should return error", func() { @@ -222,7 +222,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_VIEW}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) }) @@ -257,7 +257,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, TeamId: 1, Permission: m.PERMISSION_VIEW}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) }) @@ -285,7 +285,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, Role: &r, Permission: m.PERMISSION_EDIT}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) Convey("When trying to update dashboard permissions with everyone with editor role can view permission should return error", func() { @@ -293,7 +293,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, Role: &r, Permission: m.PERMISSION_VIEW}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) }) @@ -329,7 +329,7 @@ func TestGuardian(t *testing.T) { {OrgId: 1, DashboardId: 3, Role: &r, Permission: m.PERMISSION_VIEW}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) - So(err, ShouldEqual, ErrGuardianOverrideLowerPresedence) + So(err, ShouldEqual, ErrGuardianOverride) }) }) }) From 71a0a298bf7a5010bd9b6dfdebc692d3ea631098 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Tue, 27 Feb 2018 16:04:26 +0100 Subject: [PATCH 5/6] permissions: remove client validation and handle server validation --- .../Permissions/AddPermissions.jest.tsx | 2 +- .../components/Permissions/AddPermissions.tsx | 8 -- .../PermissionsStore/PermissionsStore.jest.ts | 75 +++---------------- .../PermissionsStore/PermissionsStore.ts | 51 +++++-------- 4 files changed, 29 insertions(+), 107 deletions(-) diff --git a/public/app/core/components/Permissions/AddPermissions.jest.tsx b/public/app/core/components/Permissions/AddPermissions.jest.tsx index 9c01eee70b1..fe97c4c7e62 100644 --- a/public/app/core/components/Permissions/AddPermissions.jest.tsx +++ b/public/app/core/components/Permissions/AddPermissions.jest.tsx @@ -17,7 +17,7 @@ describe('AddPermissions', () => { ]) ); - backendSrv.post = jest.fn(); + backendSrv.post = jest.fn(() => Promise.resolve({})); store = RootStore.create( {}, diff --git a/public/app/core/components/Permissions/AddPermissions.tsx b/public/app/core/components/Permissions/AddPermissions.tsx index 94afa7c1180..07ccfdbbef5 100644 --- a/public/app/core/components/Permissions/AddPermissions.tsx +++ b/public/app/core/components/Permissions/AddPermissions.tsx @@ -135,14 +135,6 @@ class AddPermissions extends Component { - {permissions.error ? ( -
- - - {permissions.error} - -
- ) : null} ); } diff --git a/public/app/stores/PermissionsStore/PermissionsStore.jest.ts b/public/app/stores/PermissionsStore/PermissionsStore.jest.ts index f7332516bef..c3bc6016e50 100644 --- a/public/app/stores/PermissionsStore/PermissionsStore.jest.ts +++ b/public/app/stores/PermissionsStore/PermissionsStore.jest.ts @@ -1,10 +1,10 @@ -import { PermissionsStore, aclTypeValues } from './PermissionsStore'; +import { PermissionsStore } from './PermissionsStore'; import { backendSrv } from 'test/mocks/common'; describe('PermissionsStore', () => { let store; - beforeEach(() => { + beforeEach(async () => { backendSrv.get.mockReturnValue( Promise.resolve([ { id: 2, dashboardId: 1, role: 'Viewer', permission: 1, permissionName: 'View' }, @@ -20,7 +20,7 @@ describe('PermissionsStore', () => { ]) ); - backendSrv.post = jest.fn(); + backendSrv.post = jest.fn(() => Promise.resolve({})); store = PermissionsStore.create( { @@ -32,14 +32,14 @@ describe('PermissionsStore', () => { } ); - return store.load(1, false, false); + await store.load(1, false, false); }); - it('should save update on permission change', () => { + it('should save update on permission change', async () => { expect(store.items[0].permission).toBe(1); expect(store.items[0].permissionName).toBe('View'); - store.updatePermissionOnIndex(0, 2, 'Edit'); + await store.updatePermissionOnIndex(0, 2, 'Edit'); expect(store.items[0].permission).toBe(2); expect(store.items[0].permissionName).toBe('Edit'); @@ -47,69 +47,18 @@ describe('PermissionsStore', () => { expect(backendSrv.post.mock.calls[0][0]).toBe('/api/dashboards/id/1/permissions'); }); - it('should save removed permissions automatically', () => { + it('should save removed permissions automatically', async () => { expect(store.items.length).toBe(3); - store.removeStoreItem(2); + await store.removeStoreItem(2); expect(store.items.length).toBe(2); expect(backendSrv.post.mock.calls.length).toBe(1); expect(backendSrv.post.mock.calls[0][0]).toBe('/api/dashboards/id/1/permissions'); }); - describe('when duplicate team permissions are added', () => { - beforeEach(() => { - const newItem = { - teamId: 10, - team: 'tester-team', - permission: 1, - dashboardId: 1, - }; - store.resetNewType(); - store.newItem.setTeam(newItem.teamId, newItem.team); - store.newItem.setPermission(newItem.permission); - store.addStoreItem(); - - store.newItem.setTeam(newItem.teamId, newItem.team); - store.newItem.setPermission(newItem.permission); - store.addStoreItem(); - }); - - it('should return a validation error', () => { - expect(store.items.length).toBe(4); - expect(store.error).toBe('This permission exists already.'); - expect(backendSrv.post.mock.calls.length).toBe(1); - }); - }); - - describe('when duplicate user permissions are added', () => { - beforeEach(() => { - expect(store.items.length).toBe(3); - const newItem = { - userId: 10, - userLogin: 'tester1', - permission: 1, - dashboardId: 1, - }; - store.setNewType(aclTypeValues.USER.value); - store.newItem.setUser(newItem.userId, newItem.userLogin); - store.newItem.setPermission(newItem.permission); - store.addStoreItem(); - store.setNewType(aclTypeValues.USER.value); - store.newItem.setUser(newItem.userId, newItem.userLogin); - store.newItem.setPermission(newItem.permission); - store.addStoreItem(); - }); - - it('should return a validation error', () => { - expect(store.items.length).toBe(4); - expect(store.error).toBe('This permission exists already.'); - expect(backendSrv.post.mock.calls.length).toBe(1); - }); - }); - describe('when one inherited and one not inherited team permission are added', () => { - beforeEach(() => { + beforeEach(async () => { const overridingItemForChildDashboard = { team: 'MyTestTeam', dashboardId: 1, @@ -120,11 +69,7 @@ describe('PermissionsStore', () => { store.resetNewType(); store.newItem.setTeam(overridingItemForChildDashboard.teamId, overridingItemForChildDashboard.team); store.newItem.setPermission(overridingItemForChildDashboard.permission); - store.addStoreItem(); - }); - - it('should allowing overriding the inherited permission and not throw a validation error', () => { - expect(store.error).toBe(null); + await store.addStoreItem(); }); it('should add new overriding permission', () => { diff --git a/public/app/stores/PermissionsStore/PermissionsStore.ts b/public/app/stores/PermissionsStore/PermissionsStore.ts index 88b26a2886f..79df593f06e 100644 --- a/public/app/stores/PermissionsStore/PermissionsStore.ts +++ b/public/app/stores/PermissionsStore/PermissionsStore.ts @@ -1,8 +1,6 @@ import { types, getEnv, flow } from 'mobx-state-tree'; import { PermissionsStoreItem } from './PermissionsStoreItem'; -const duplicateError = 'This permission exists already.'; - export const permissionOptions = [ { value: 1, label: 'View', description: 'Can view dashboards.' }, { value: 2, label: 'Edit', description: 'Can add, edit and delete dashboards.' }, @@ -75,7 +73,6 @@ export const PermissionsStore = types isFolder: types.maybe(types.boolean), dashboardId: types.maybe(types.number), items: types.optional(types.array(PermissionsStoreItem), []), - error: types.maybe(types.string), originalItems: types.optional(types.array(PermissionsStoreItem), []), newType: types.optional(types.string, defaultNewType), newItem: types.maybe(NewPermissionsItem), @@ -88,7 +85,6 @@ export const PermissionsStore = types return isDuplicate(it, item); }); if (dupe) { - self.error = duplicateError; return false; } @@ -96,8 +92,7 @@ export const PermissionsStore = types }, })) .actions(self => { - const resetNewType = () => { - self.error = null; + const resetNewTypeInternal = () => { self.newItem = NewPermissionsItem.create(); }; @@ -115,11 +110,9 @@ export const PermissionsStore = types self.items = items; self.originalItems = items; self.fetching = false; - self.error = null; }), addStoreItem: flow(function* addStoreItem() { - self.error = null; let item = { type: self.newItem.type, permission: self.newItem.permission, @@ -147,19 +140,21 @@ export const PermissionsStore = types throw Error('Unknown type: ' + self.newItem.type); } - if (!self.isValid(item)) { - return undefined; - } + const updatedItems = self.items.peek(); + const newItem = prepareItem(item, self.dashboardId, self.isFolder, self.isInRoot); + updatedItems.push(newItem); - self.items.push(prepareItem(item, self.dashboardId, self.isFolder, self.isInRoot)); - resetNewType(); - return updateItems(self); + try { + yield updateItems(self, updatedItems); + self.items.push(newItem); + resetNewTypeInternal(); + } catch {} + yield Promise.resolve(); }), removeStoreItem: flow(function* removeStoreItem(idx: number) { - self.error = null; self.items.splice(idx, 1); - return updateItems(self); + yield updateItems(self, self.items.peek()); }), updatePermissionOnIndex: flow(function* updatePermissionOnIndex( @@ -167,9 +162,8 @@ export const PermissionsStore = types permission: number, permissionName: string ) { - self.error = null; self.items[idx].updatePermission(permission, permissionName); - return updateItems(self); + yield updateItems(self, self.items.peek()); }), setNewType(newType: string) { @@ -177,7 +171,7 @@ export const PermissionsStore = types }, resetNewType() { - resetNewType(); + resetNewTypeInternal(); }, toggleAddPermissions() { @@ -190,12 +184,10 @@ export const PermissionsStore = types }; }); -const updateItems = self => { - self.error = null; - +const updateItems = (self, items) => { const backendSrv = getEnv(self).backendSrv; const updated = []; - for (let item of self.items) { + for (let item of items) { if (item.inherited) { continue; } @@ -208,16 +200,9 @@ const updateItems = self => { }); } - let res; - try { - res = backendSrv.post(`/api/dashboards/id/${self.dashboardId}/permissions`, { - items: updated, - }); - } catch (error) { - self.error = error; - } - - return res; + return backendSrv.post(`/api/dashboards/id/${self.dashboardId}/permissions`, { + items: updated, + }); }; const prepareServerResponse = (response, dashboardId: number, isFolder: boolean, isInRoot: boolean) => { From f44e476580d7b4fe0ca06be802dde4689131820e Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Wed, 28 Feb 2018 08:48:28 +0100 Subject: [PATCH 6/6] permissions: fix validation of permissions before update Did a bad pointer comparison so extended the tests for duplicate permissions. --- pkg/models/dashboard_acl.go | 21 +++++++++ pkg/services/guardian/guardian.go | 33 ++++++++++---- pkg/services/guardian/guardian_test.go | 62 +++++++++++++++++++++++--- 3 files changed, 103 insertions(+), 13 deletions(-) diff --git a/pkg/models/dashboard_acl.go b/pkg/models/dashboard_acl.go index 1fbd2b451b9..5b91b2a70b4 100644 --- a/pkg/models/dashboard_acl.go +++ b/pkg/models/dashboard_acl.go @@ -68,6 +68,27 @@ type DashboardAclInfoDTO struct { Url string `json:"url"` } +func (dto *DashboardAclInfoDTO) hasSameRoleAs(other *DashboardAclInfoDTO) bool { + if dto.Role == nil || other.Role == nil { + return false + } + + return dto.UserId <= 0 && dto.TeamId <= 0 && dto.UserId == other.UserId && dto.TeamId == other.TeamId && *dto.Role == *other.Role +} + +func (dto *DashboardAclInfoDTO) hasSameUserAs(other *DashboardAclInfoDTO) bool { + return dto.UserId > 0 && dto.UserId == other.UserId +} + +func (dto *DashboardAclInfoDTO) hasSameTeamAs(other *DashboardAclInfoDTO) bool { + return dto.TeamId > 0 && dto.TeamId == other.TeamId +} + +// IsDuplicateOf returns true if other item has same role, same user or same team +func (dto *DashboardAclInfoDTO) IsDuplicateOf(other *DashboardAclInfoDTO) bool { + return dto.hasSameRoleAs(other) || dto.hasSameUserAs(other) || dto.hasSameTeamAs(other) +} + // // COMMANDS // diff --git a/pkg/services/guardian/guardian.go b/pkg/services/guardian/guardian.go index 54b5386f40d..811b38cac86 100644 --- a/pkg/services/guardian/guardian.go +++ b/pkg/services/guardian/guardian.go @@ -10,7 +10,7 @@ import ( ) var ( - ErrGuardianPermissionExists = errors.New("This permission already exists") + ErrGuardianPermissionExists = errors.New("Permission already exists") ErrGuardianOverride = errors.New("You can only override a permission to be higher") ) @@ -127,17 +127,23 @@ func (g *dashboardGuardianImpl) checkAcl(permission m.PermissionType, acl []*m.D func (g *dashboardGuardianImpl) CheckPermissionBeforeUpdate(permission m.PermissionType, updatePermissions []*m.DashboardAcl) (bool, error) { acl := []*m.DashboardAclInfoDTO{} + adminRole := m.ROLE_ADMIN + everyoneWithAdminRole := &m.DashboardAclInfoDTO{DashboardId: g.dashId, UserId: 0, TeamId: 0, Role: &adminRole, Permission: m.PERMISSION_ADMIN} + // validate that duplicate permissions don't exists for _, p := range updatePermissions { + aclItem := &m.DashboardAclInfoDTO{DashboardId: p.DashboardId, UserId: p.UserId, TeamId: p.TeamId, Role: p.Role, Permission: p.Permission} + if aclItem.IsDuplicateOf(everyoneWithAdminRole) { + return false, ErrGuardianPermissionExists + } + for _, a := range acl { - if (a.UserId <= 0 && a.TeamId <= 0 && a.UserId == p.UserId && a.TeamId == p.TeamId && a.Role == p.Role) || - (a.UserId > 0 && a.UserId == p.UserId) || - (a.TeamId > 0 && a.TeamId == p.TeamId) { + if a.IsDuplicateOf(aclItem) { return false, ErrGuardianPermissionExists } } - acl = append(acl, &m.DashboardAclInfoDTO{DashboardId: p.DashboardId, UserId: p.UserId, TeamId: p.TeamId, Role: p.Role, Permission: p.Permission}) + acl = append(acl, aclItem) } existingPermissions, err := g.GetAcl() @@ -145,15 +151,19 @@ func (g *dashboardGuardianImpl) CheckPermissionBeforeUpdate(permission m.Permiss return false, err } + // validate overridden permissions to be higher for _, a := range acl { for _, existingPerm := range existingPermissions { + // handle default permissions + if existingPerm.DashboardId == -1 { + existingPerm.DashboardId = g.dashId + } + if a.DashboardId == existingPerm.DashboardId { continue } - if (a.UserId <= 0 && a.TeamId <= 0 && a.UserId == existingPerm.UserId && a.TeamId == existingPerm.TeamId && *a.Role == *existingPerm.Role && a.Permission <= existingPerm.Permission) || - (a.UserId > 0 && a.UserId == existingPerm.UserId && a.Permission <= existingPerm.Permission) || - (a.TeamId > 0 && a.TeamId == existingPerm.TeamId && a.Permission <= existingPerm.Permission) { + if a.IsDuplicateOf(existingPerm) && a.Permission <= existingPerm.Permission { return false, ErrGuardianOverride } } @@ -177,6 +187,13 @@ func (g *dashboardGuardianImpl) GetAcl() ([]*m.DashboardAclInfoDTO, error) { return nil, err } + for _, a := range query.Result { + // handle default permissions + if a.DashboardId == -1 { + a.DashboardId = g.dashId + } + } + g.acl = query.Result return g.acl, nil } diff --git a/pkg/services/guardian/guardian_test.go b/pkg/services/guardian/guardian_test.go index 0da14a51c5c..bb7e6bd1a72 100644 --- a/pkg/services/guardian/guardian_test.go +++ b/pkg/services/guardian/guardian_test.go @@ -23,7 +23,7 @@ func TestGuardian(t *testing.T) { So(canView, ShouldBeTrue) Convey("When trying to update permissions", func() { - Convey("With duplicate user/role permissions should return error", func() { + Convey("With duplicate user permissions should return error", func() { p := []*m.DashboardAcl{ {OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_VIEW}, {OrgId: 1, DashboardId: 1, UserId: 1, Permission: m.PERMISSION_ADMIN}, @@ -32,7 +32,7 @@ func TestGuardian(t *testing.T) { So(err, ShouldEqual, ErrGuardianPermissionExists) }) - Convey("With duplicate team/role permissions should return error", func() { + Convey("With duplicate team permissions should return error", func() { p := []*m.DashboardAcl{ {OrgId: 1, DashboardId: 1, TeamId: 1, Permission: m.PERMISSION_VIEW}, {OrgId: 1, DashboardId: 1, TeamId: 1, Permission: m.PERMISSION_ADMIN}, @@ -41,14 +41,66 @@ func TestGuardian(t *testing.T) { So(err, ShouldEqual, ErrGuardianPermissionExists) }) - Convey("With duplicate everyone/role permissions should return error", func() { + Convey("With duplicate everyone with editor role permission should return error", func() { + r := m.ROLE_EDITOR p := []*m.DashboardAcl{ - {OrgId: 1, DashboardId: 1, Permission: m.PERMISSION_VIEW}, - {OrgId: 1, DashboardId: 1, Permission: m.PERMISSION_ADMIN}, + {OrgId: 1, DashboardId: 1, Role: &r, Permission: m.PERMISSION_VIEW}, + {OrgId: 1, DashboardId: 1, Role: &r, Permission: m.PERMISSION_ADMIN}, } _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) So(err, ShouldEqual, ErrGuardianPermissionExists) }) + + Convey("With duplicate everyone with viewer role permission should return error", func() { + r := m.ROLE_VIEWER + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 1, Role: &r, Permission: m.PERMISSION_VIEW}, + {OrgId: 1, DashboardId: 1, Role: &r, Permission: m.PERMISSION_ADMIN}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianPermissionExists) + }) + + Convey("With everyone with admin role permission should return error", func() { + r := m.ROLE_ADMIN + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 1, Role: &r, Permission: m.PERMISSION_ADMIN}, + } + _, err := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(err, ShouldEqual, ErrGuardianPermissionExists) + }) + }) + + Convey("Given default permissions", func() { + editor := m.ROLE_EDITOR + viewer := m.ROLE_VIEWER + existingPermissions := []*m.DashboardAclInfoDTO{ + {OrgId: 1, DashboardId: -1, Role: &editor, Permission: m.PERMISSION_EDIT}, + {OrgId: 1, DashboardId: -1, Role: &viewer, Permission: m.PERMISSION_VIEW}, + } + + bus.AddHandler("test", func(query *m.GetDashboardAclInfoListQuery) error { + query.Result = existingPermissions + return nil + }) + + Convey("When trying to update dashboard permissions without everyone with role editor can edit should be allowed", func() { + r := m.ROLE_VIEWER + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 1, Role: &r, Permission: m.PERMISSION_VIEW}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeTrue) + }) + + Convey("When trying to update dashboard permissions without everyone with role viewer can view should be allowed", func() { + r := m.ROLE_EDITOR + p := []*m.DashboardAcl{ + {OrgId: 1, DashboardId: 1, Role: &r, Permission: m.PERMISSION_EDIT}, + } + ok, _ := sc.g.CheckPermissionBeforeUpdate(m.PERMISSION_ADMIN, p) + So(ok, ShouldBeTrue) + }) }) Convey("Given parent folder has user admin permission", func() {