Access control: SQL filtering for annotation listing (#47467)

* pass in user to attribute scope resolver

* add SQL filter to annotation listing

* check annotation FGAC permissions before exposing them for commenting

* remove the requirement to be able to list all annotations from annotation listing endpoint

* adding tests for annotation listing

* remove changes that got moved to a different PR

* unused var

* Update pkg/services/sqlstore/annotation.go

Co-authored-by: Ezequiel Victorero <evictorero@gmail.com>

* remove unneeded check

* remove unneeded check

* undo accidental change

* undo accidental change

* doc update

* move tests

* redo the approach for passing the user in for scope resolution

* accidental change

* cleanup

* error handling

Co-authored-by: Ezequiel Victorero <evictorero@gmail.com>

conf/defaults.ini

@@ -1131,7 +1131,7 @@ license_path =
 # enable = feature1,feature2
 enable =
 
-# The new prometheus visual query builder 
+# The new prometheus visual query builder
 promQueryBuilder = true
 
 # feature1 = true

docs/sources/http_api/annotations.md

@@ -20,9 +20,9 @@ This is the API documentation for the new Grafana Annotations feature released i
 
 See note in the [introduction]({{< ref "#annotations-api" >}}) for an explanation.
 
-| Action           | Scope          |
-| ---------------- | -------------- |
-| annotations:read | annotations:\* |
+| Action           | Scope                   |
+| ---------------- | ----------------------- |
+| annotations:read | annotations:type:<type> |
 
 **Example Request**:
 

pkg/api/annotations.go

@@ -20,17 +20,18 @@ import (
 
 func (hs *HTTPServer) GetAnnotations(c *models.ReqContext) response.Response {
 	query := &annotations.ItemQuery{
-		From:        c.QueryInt64("from"),
-		To:          c.QueryInt64("to"),
-		OrgId:       c.OrgId,
-		UserId:      c.QueryInt64("userId"),
-		AlertId:     c.QueryInt64("alertId"),
-		DashboardId: c.QueryInt64("dashboardId"),
-		PanelId:     c.QueryInt64("panelId"),
-		Limit:       c.QueryInt64("limit"),
-		Tags:        c.QueryStrings("tags"),
-		Type:        c.Query("type"),
-		MatchAny:    c.QueryBool("matchAny"),
+		From:         c.QueryInt64("from"),
+		To:           c.QueryInt64("to"),
+		OrgId:        c.OrgId,
+		UserId:       c.QueryInt64("userId"),
+		AlertId:      c.QueryInt64("alertId"),
+		DashboardId:  c.QueryInt64("dashboardId"),
+		PanelId:      c.QueryInt64("panelId"),
+		Limit:        c.QueryInt64("limit"),
+		Tags:         c.QueryStrings("tags"),
+		Type:         c.Query("type"),
+		MatchAny:     c.QueryBool("matchAny"),
+		SignedInUser: c.SignedInUser,
 	}
 
 	repo := annotations.GetRepository()
@@ -192,7 +193,7 @@ func (hs *HTTPServer) UpdateAnnotation(c *models.ReqContext) response.Response {
 
 	repo := annotations.GetRepository()
 
-	annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.OrgId)
+	annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.SignedInUser)
 	if resp != nil {
 		return resp
 	}
@@ -239,7 +240,7 @@ func (hs *HTTPServer) PatchAnnotation(c *models.ReqContext) response.Response {
 
 	repo := annotations.GetRepository()
 
-	annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.OrgId)
+	annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.SignedInUser)
 	if resp != nil {
 		return resp
 	}
@@ -310,7 +311,7 @@ func (hs *HTTPServer) MassDeleteAnnotations(c *models.ReqContext) response.Respo
 		var dashboardId int64
 
 		if cmd.AnnotationId != 0 {
-			annotation, respErr := findAnnotationByID(c.Req.Context(), repo, cmd.AnnotationId, c.OrgId)
+			annotation, respErr := findAnnotationByID(c.Req.Context(), repo, cmd.AnnotationId, c.SignedInUser)
 			if respErr != nil {
 				return respErr
 			}
@@ -358,7 +359,7 @@ func (hs *HTTPServer) DeleteAnnotationByID(c *models.ReqContext) response.Respon
 
 	repo := annotations.GetRepository()
 
-	annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.OrgId)
+	annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.SignedInUser)
 	if resp != nil {
 		return resp
 	}
@@ -400,8 +401,13 @@ func canSaveOrganizationAnnotation(c *models.ReqContext) bool {
 	return c.SignedInUser.HasRole(models.ROLE_EDITOR)
 }
 
-func findAnnotationByID(ctx context.Context, repo annotations.Repository, annotationID int64, orgID int64) (*annotations.ItemDTO, response.Response) {
-	items, err := repo.Find(ctx, &annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgID})
+func findAnnotationByID(ctx context.Context, repo annotations.Repository, annotationID int64, user *models.SignedInUser) (*annotations.ItemDTO, response.Response) {
+	query := &annotations.ItemQuery{
+		AnnotationId: annotationID,
+		OrgId:        user.OrgId,
+		SignedInUser: user,
+	}
+	items, err := repo.Find(ctx, query)
 
 	if err != nil {
 		return nil, response.Error(500, "Failed to find annotation", err)
@@ -446,9 +452,21 @@ func AnnotationTypeScopeResolver() (string, accesscontrol.AttributeScopeResolveF
 			return "", accesscontrol.ErrInvalidScope
 		}
 
-		annotation, resp := findAnnotationByID(ctx, annotations.GetRepository(), int64(annotationId), orgID)
+		// tempUser is used to resolve annotation type.
+		// The annotation doesn't get returned to the real user, so real user's permissions don't matter here.
+		tempUser := &models.SignedInUser{
+			OrgId: orgID,
+			Permissions: map[int64]map[string][]string{
+				orgID: {
+					accesscontrol.ActionDashboardsRead:  {accesscontrol.ScopeDashboardsAll},
+					accesscontrol.ActionAnnotationsRead: {accesscontrol.ScopeAnnotationsAll},
+				},
+			},
+		}
+
+		annotation, resp := findAnnotationByID(ctx, annotations.GetRepository(), int64(annotationId), tempUser)
 		if resp != nil {
-			return "", err
+			return "", errors.New("could not resolve annotation type")
 		}
 
 		if annotation.GetType() == annotations.Organization {

pkg/api/api.go

@@ -446,7 +446,7 @@ func (hs *HTTPServer) registerRoutes() {
 			orgRoute.Get("/lookup", routing.Wrap(hs.GetAlertNotificationLookup))
 		})
 
-		apiRoute.Get("/annotations", authorize(reqSignedIn, ac.EvalPermission(ac.ActionAnnotationsRead, ac.ScopeAnnotationsAll)), routing.Wrap(hs.GetAnnotations))
+		apiRoute.Get("/annotations", authorize(reqSignedIn, ac.EvalPermission(ac.ActionAnnotationsRead)), routing.Wrap(hs.GetAnnotations))
 		apiRoute.Post("/annotations/mass-delete", authorize(reqOrgAdmin, ac.EvalPermission(ac.ActionAnnotationsDelete)), routing.Wrap(hs.MassDeleteAnnotations))
 
 		apiRoute.Group("/annotations", func(annotationsRoute routing.RouteRegister) {

pkg/services/accesscontrol/filter.go

@@ -43,7 +43,7 @@ func Filter(user *models.SignedInUser, sqlID, prefix string, actions ...string)
 	wildcards := 0
 	result := make(map[interface{}]int)
 	for _, a := range actions {
-		ids, hasWildcard := parseScopes(prefix, user.Permissions[user.OrgId][a])
+		ids, hasWildcard := ParseScopes(prefix, user.Permissions[user.OrgId][a])
 		if hasWildcard {
 			wildcards += 1
 			continue
@@ -84,7 +84,7 @@ func Filter(user *models.SignedInUser, sqlID, prefix string, actions ...string)
 	return SQLFilter{query.String(), ids}, nil
 }
 
-func parseScopes(prefix string, scopes []string) (ids map[interface{}]struct{}, hasWildcard bool) {
+func ParseScopes(prefix string, scopes []string) (ids map[interface{}]struct{}, hasWildcard bool) {
 	ids = make(map[interface{}]struct{})
 
 	rootPrefix, attributePrefix, ok := extractPrefixes(prefix)

pkg/services/accesscontrol/models.go

@@ -4,6 +4,8 @@ import (
 	"encoding/json"
 	"strings"
 	"time"
+
+	"github.com/grafana/grafana/pkg/services/annotations"
 )
 
 // RoleRegistration stores a role and its assignments to built-in roles
@@ -379,8 +381,8 @@ var (
 	ScopeAnnotationsProvider         = NewScopeProvider(ScopeAnnotationsRoot)
 	ScopeAnnotationsAll              = ScopeAnnotationsProvider.GetResourceAllScope()
 	ScopeAnnotationsID               = Scope(ScopeAnnotationsRoot, "id", Parameter(":annotationId"))
-	ScopeAnnotationsTypeDashboard    = ScopeAnnotationsProvider.GetResourceScopeType("dashboard")
-	ScopeAnnotationsTypeOrganization = ScopeAnnotationsProvider.GetResourceScopeType("organization")
+	ScopeAnnotationsTypeDashboard    = ScopeAnnotationsProvider.GetResourceScopeType(annotations.Dashboard.String())
+	ScopeAnnotationsTypeOrganization = ScopeAnnotationsProvider.GetResourceScopeType(annotations.Organization.String())
 )
 
 const RoleGrafanaAdmin = "Grafana Admin"

pkg/services/annotations/annotations.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 
 	"github.com/grafana/grafana/pkg/components/simplejson"
+	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/setting"
 )
 
@@ -37,6 +38,7 @@ type ItemQuery struct {
 	Tags         []string `json:"tags"`
 	Type         string   `json:"type"`
 	MatchAny     bool     `json:"matchAny"`
+	SignedInUser *models.SignedInUser
 
 	Limit int64 `json:"limit"`
 }
@@ -152,6 +154,17 @@ const (
 	Dashboard
 )
 
+func (a annotationType) String() string {
+	switch a {
+	case Organization:
+		return "organization"
+	case Dashboard:
+		return "dashboard"
+	default:
+		return ""
+	}
+}
+
 func (annotation *ItemDTO) GetType() annotationType {
 	if annotation.DashboardId != 0 {
 		return Dashboard

pkg/services/comments/commentmodel/permissions.go

@@ -62,7 +62,7 @@ func (c *PermissionChecker) CheckReadPermissions(ctx context.Context, orgId int6
 		if err != nil {
 			return false, nil
 		}
-		items, err := repo.Find(ctx, &annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgId})
+		items, err := repo.Find(ctx, &annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgId, SignedInUser: signedInUser})
 		if err != nil || len(items) != 1 {
 			return false, nil
 		}
@@ -109,7 +109,7 @@ func (c *PermissionChecker) CheckWritePermissions(ctx context.Context, orgId int
 		if err != nil {
 			return false, nil
 		}
-		items, err := repo.Find(ctx, &annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgId})
+		items, err := repo.Find(ctx, &annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgId, SignedInUser: signedInUser})
 		if err != nil || len(items) != 1 {
 			return false, nil
 		}

pkg/services/sqlstore/annotation.go

@@ -9,7 +9,11 @@ import (
 	"time"
 
 	"github.com/grafana/grafana/pkg/models"
+	ac "github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/services/annotations"
+	"github.com/grafana/grafana/pkg/services/featuremgmt"
+	"github.com/grafana/grafana/pkg/services/sqlstore/permissions"
+	"github.com/grafana/grafana/pkg/services/sqlstore/searchstore"
 )
 
 // Update the item so that EpochEnd >= Epoch
@@ -33,6 +37,10 @@ type SQLAnnotationRepo struct {
 	sql *SQLStore
 }
 
+func NewSQLAnnotationRepo(sql *SQLStore) SQLAnnotationRepo {
+	return SQLAnnotationRepo{sql: sql}
+}
+
 func (r *SQLAnnotationRepo) Save(item *annotations.Item) error {
 	return inTransaction(func(sess *DBSession) error {
 		tags := models.ParseTagPairs(item.Tags)
@@ -221,6 +229,15 @@ func (r *SQLAnnotationRepo) Find(ctx context.Context, query *annotations.ItemQue
 			}
 		}
 
+		if r.sql.Cfg.IsFeatureToggleEnabled(featuremgmt.FlagAccesscontrol) {
+			acFilter, acArgs, err := getAccessControlFilter(query.SignedInUser)
+			if err != nil {
+				return err
+			}
+			sql.WriteString(fmt.Sprintf(" AND (%s)", acFilter))
+			params = append(params, acArgs...)
+		}
+
 		if query.Limit == 0 {
 			query.Limit = 100
 		}
@@ -239,6 +256,37 @@ func (r *SQLAnnotationRepo) Find(ctx context.Context, query *annotations.ItemQue
 	return items, err
 }
 
+func getAccessControlFilter(user *models.SignedInUser) (string, []interface{}, error) {
+	if user == nil || user.Permissions[user.OrgId] == nil {
+		return "", nil, errors.New("missing permissions")
+	}
+	scopes, has := user.Permissions[user.OrgId][ac.ActionAnnotationsRead]
+	if !has {
+		return "", nil, errors.New("missing permissions")
+	}
+	types, hasWildcardScope := ac.ParseScopes(ac.ScopeAnnotationsProvider.GetResourceScopeType(""), scopes)
+	if hasWildcardScope {
+		types = map[interface{}]struct{}{annotations.Dashboard.String(): {}, annotations.Organization.String(): {}}
+	}
+
+	var filters []string
+	var params []interface{}
+	for t := range types {
+		// annotation read permission with scope annotations:type:organization allows listing annotations that are not associated with a dashboard
+		if t == annotations.Organization.String() {
+			filters = append(filters, "a.dashboard_id = 0")
+		}
+		// annotation read permission with scope annotations:type:dashboard allows listing annotations from dashboards which the user can view
+		if t == annotations.Dashboard.String() {
+			dashboardFilter, dashboardParams := permissions.NewAccessControlDashboardPermissionFilter(user, models.PERMISSION_VIEW, searchstore.TypeDashboard).Where()
+			filter := fmt.Sprintf("a.dashboard_id IN(SELECT id FROM dashboard WHERE %s)", dashboardFilter)
+			filters = append(filters, filter)
+			params = dashboardParams
+		}
+	}
+	return strings.Join(filters, " OR "), params, nil
+}
+
 func (r *SQLAnnotationRepo) Delete(ctx context.Context, params *annotations.DeleteParams) error {
 	return r.sql.WithTransactionalDbSession(ctx, func(sess *DBSession) error {
 		var (

pkg/services/sqlstore/annotation_test.go

@@ -1,27 +1,32 @@
 //go:build integration
 // +build integration
 
-package sqlstore
+package sqlstore_test
 
 import (
 	"context"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
+	"fmt"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/grafana/pkg/components/simplejson"
+	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/services/annotations"
+	dashboardstore "github.com/grafana/grafana/pkg/services/dashboards/database"
+	"github.com/grafana/grafana/pkg/services/featuremgmt"
+	"github.com/grafana/grafana/pkg/services/sqlstore"
 )
 
 func TestAnnotations(t *testing.T) {
-	mockTimeNow()
-	defer resetTimeNow()
-
-	repo := SQLAnnotationRepo{}
-	repo.sql = InitTestDB(t)
+	sql := sqlstore.InitTestDB(t)
+	repo := sqlstore.NewSQLAnnotationRepo(sql)
 
 	t.Run("Testing annotation create, read, update and delete", func(t *testing.T) {
 		t.Cleanup(func() {
-			err := repo.sql.WithDbSession(context.Background(), func(dbSession *DBSession) error {
+			err := sql.WithDbSession(context.Background(), func(dbSession *sqlstore.DBSession) error {
 				_, err := dbSession.Exec("DELETE FROM annotation WHERE 1=1")
 				if err != nil {
 					return err
@@ -331,3 +336,136 @@ func TestAnnotations(t *testing.T) {
 		})
 	})
 }
+
+func TestAnnotationListingWithFGAC(t *testing.T) {
+	sql := sqlstore.InitTestDB(t)
+	sql.Cfg.IsFeatureToggleEnabled = func(key string) bool {
+		return key == featuremgmt.FlagAccesscontrol
+	}
+	repo := sqlstore.NewSQLAnnotationRepo(sql)
+	dashboardStore := dashboardstore.ProvideDashboardStore(sql)
+
+	testDashboard1 := models.SaveDashboardCommand{
+		UserId: 1,
+		OrgId:  1,
+		Dashboard: simplejson.NewFromAny(map[string]interface{}{
+			"title": "Dashboard 1",
+		}),
+	}
+	dashboard, err := dashboardStore.SaveDashboard(testDashboard1)
+	require.NoError(t, err)
+	dash1UID := dashboard.Uid
+
+	testDashboard2 := models.SaveDashboardCommand{
+		UserId: 1,
+		OrgId:  1,
+		Dashboard: simplejson.NewFromAny(map[string]interface{}{
+			"title": "Dashboard 2",
+		}),
+	}
+	_, err = dashboardStore.SaveDashboard(testDashboard2)
+	require.NoError(t, err)
+
+	dash1Annotation := &annotations.Item{
+		OrgId:       1,
+		DashboardId: 1,
+		Epoch:       10,
+	}
+	err = repo.Save(dash1Annotation)
+	require.NoError(t, err)
+
+	dash2Annotation := &annotations.Item{
+		OrgId:       1,
+		DashboardId: 2,
+		Epoch:       10,
+	}
+	err = repo.Save(dash2Annotation)
+	require.NoError(t, err)
+
+	organizationAnnotation := &annotations.Item{
+		OrgId: 1,
+		Epoch: 10,
+	}
+	err = repo.Save(organizationAnnotation)
+	require.NoError(t, err)
+
+	user := &models.SignedInUser{
+		UserId: 1,
+		OrgId:  1,
+	}
+
+	type testStruct struct {
+		description           string
+		permissions           map[string][]string
+		expectedAnnotationIds []int64
+		expectedError         bool
+	}
+
+	testCases := []testStruct{
+		{
+			description: "Should find all annotations when has permissions to list all annotations and read all dashboards",
+			permissions: map[string][]string{
+				accesscontrol.ActionAnnotationsRead: {accesscontrol.ScopeAnnotationsAll},
+				accesscontrol.ActionDashboardsRead:  {accesscontrol.ScopeDashboardsAll},
+			},
+			expectedAnnotationIds: []int64{dash1Annotation.Id, dash2Annotation.Id, organizationAnnotation.Id},
+		},
+		{
+			description: "Should find all dashboard annotations",
+			permissions: map[string][]string{
+				accesscontrol.ActionAnnotationsRead: {accesscontrol.ScopeAnnotationsTypeDashboard},
+				accesscontrol.ActionDashboardsRead:  {accesscontrol.ScopeDashboardsAll},
+			},
+			expectedAnnotationIds: []int64{dash1Annotation.Id, dash2Annotation.Id},
+		},
+		{
+			description: "Should find only annotations from dashboards that user can read",
+			permissions: map[string][]string{
+				accesscontrol.ActionAnnotationsRead: {accesscontrol.ScopeAnnotationsTypeDashboard},
+				accesscontrol.ActionDashboardsRead:  {fmt.Sprintf("dashboards:uid:%s", dash1UID)},
+			},
+			expectedAnnotationIds: []int64{dash1Annotation.Id},
+		},
+		{
+			description: "Should find no annotations if user can't view dashboards or organization annotations",
+			permissions: map[string][]string{
+				accesscontrol.ActionAnnotationsRead: {accesscontrol.ScopeAnnotationsTypeDashboard},
+			},
+			expectedAnnotationIds: []int64{},
+		},
+		{
+			description: "Should find only organization annotations",
+			permissions: map[string][]string{
+				accesscontrol.ActionAnnotationsRead: {accesscontrol.ScopeAnnotationsTypeOrganization},
+				accesscontrol.ActionDashboardsRead:  {accesscontrol.ScopeDashboardsAll},
+			},
+			expectedAnnotationIds: []int64{organizationAnnotation.Id},
+		},
+		{
+			description: "Should error if user doesn't have annotation read permissions",
+			permissions: map[string][]string{
+				accesscontrol.ActionDashboardsRead: {accesscontrol.ScopeDashboardsAll},
+			},
+			expectedError: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			user.Permissions = map[int64]map[string][]string{1: tc.permissions}
+			results, err := repo.Find(context.Background(), &annotations.ItemQuery{
+				OrgId:        1,
+				SignedInUser: user,
+			})
+			if tc.expectedError {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			assert.Len(t, results, len(tc.expectedAnnotationIds))
+			for _, r := range results {
+				assert.Contains(t, tc.expectedAnnotationIds, r.Id)
+			}
+		})
+	}
+}