From f44e476580d7b4fe0ca06be802dde4689131820e Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Wed, 28 Feb 2018 08:48:28 +0100 Subject: [PATCH] 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() {