permissions: fix validation of permissions before update

Did a bad pointer comparison so extended the tests for duplicate permissions.

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
 //

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
 }

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() {