From ef4c2672b3c75755af38100a0f56d98c158325b7 Mon Sep 17 00:00:00 2001 From: Ieva Date: Mon, 11 Apr 2022 13:18:38 +0100 Subject: [PATCH] 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 * 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 --- conf/defaults.ini | 2 +- docs/sources/http_api/annotations.md | 6 +- pkg/api/annotations.go | 56 ++++--- pkg/api/api.go | 2 +- pkg/services/accesscontrol/filter.go | 4 +- pkg/services/accesscontrol/models.go | 6 +- pkg/services/annotations/annotations.go | 13 ++ .../comments/commentmodel/permissions.go | 4 +- pkg/services/sqlstore/annotation.go | 48 ++++++ pkg/services/sqlstore/annotation_test.go | 156 +++++++++++++++++- 10 files changed, 258 insertions(+), 39 deletions(-) diff --git a/conf/defaults.ini b/conf/defaults.ini index d0ab0c1a16c..e43760be0ef 100644 --- a/conf/defaults.ini +++ b/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 diff --git a/docs/sources/http_api/annotations.md b/docs/sources/http_api/annotations.md index e784f6b1841..a925a6d6b53 100644 --- a/docs/sources/http_api/annotations.md +++ b/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: | **Example Request**: diff --git a/pkg/api/annotations.go b/pkg/api/annotations.go index 5221332c741..9fd7cc61195 100644 --- a/pkg/api/annotations.go +++ b/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 { diff --git a/pkg/api/api.go b/pkg/api/api.go index 12c7dbd4a8c..bc663b6939f 100644 --- a/pkg/api/api.go +++ b/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) { diff --git a/pkg/services/accesscontrol/filter.go b/pkg/services/accesscontrol/filter.go index 2fc40de2796..67eb5c49eaa 100644 --- a/pkg/services/accesscontrol/filter.go +++ b/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) diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index d3df60664a5..6c5c3d576c3 100644 --- a/pkg/services/accesscontrol/models.go +++ b/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" diff --git a/pkg/services/annotations/annotations.go b/pkg/services/annotations/annotations.go index 9b79c1cdeec..5dea39e5ccc 100644 --- a/pkg/services/annotations/annotations.go +++ b/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 diff --git a/pkg/services/comments/commentmodel/permissions.go b/pkg/services/comments/commentmodel/permissions.go index bdfed834f44..e603e306c24 100644 --- a/pkg/services/comments/commentmodel/permissions.go +++ b/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 } diff --git a/pkg/services/sqlstore/annotation.go b/pkg/services/sqlstore/annotation.go index 633bc3bb10d..83f3e0f319f 100644 --- a/pkg/services/sqlstore/annotation.go +++ b/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 ( diff --git a/pkg/services/sqlstore/annotation_test.go b/pkg/services/sqlstore/annotation_test.go index 4c508b7f0fc..6b66861d0cb 100644 --- a/pkg/services/sqlstore/annotation_test.go +++ b/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) + } + }) + } +}