From 4f815e3d8e85c49000f0f0c17832e3d99239f59c Mon Sep 17 00:00:00 2001 From: Ezequiel Victorero Date: Fri, 11 Feb 2022 15:43:29 -0300 Subject: [PATCH] Access control: adding FGAC to annotation GET endpoints and fixed roles (#45102) * Access control: adding FGAC to annotation GET endpoints and fixed roles Co-authored-by: Ieva --- pkg/api/annotations.go | 51 ++++++----- pkg/api/annotations_test.go | 132 ++++++++++++++++++++------- pkg/api/api.go | 4 +- pkg/api/roles.go | 19 +++- pkg/services/accesscontrol/models.go | 8 ++ 5 files changed, 157 insertions(+), 57 deletions(-) diff --git a/pkg/api/annotations.go b/pkg/api/annotations.go index 24e725f3654..449e49aeccb 100644 --- a/pkg/api/annotations.go +++ b/pkg/api/annotations.go @@ -173,10 +173,15 @@ func UpdateAnnotation(c *models.ReqContext) response.Response { repo := annotations.GetRepository() - if resp := canSave(c, repo, annotationID); resp != nil { + annotation, resp := findAnnotationByID(repo, annotationID, c.OrgId) + if resp != nil { return resp } + if canSave, err := canSaveByDashboardID(c, annotation.DashboardId); err != nil || !canSave { + return dashboardGuardianResponse(err) + } + item := annotations.Item{ OrgId: c.OrgId, UserId: c.UserId, @@ -206,24 +211,23 @@ func PatchAnnotation(c *models.ReqContext) response.Response { repo := annotations.GetRepository() - if resp := canSave(c, repo, annotationID); resp != nil { + annotation, resp := findAnnotationByID(repo, annotationID, c.OrgId) + if resp != nil { return resp } - items, err := repo.Find(&annotations.ItemQuery{AnnotationId: annotationID, OrgId: c.OrgId}) - - if err != nil || len(items) == 0 { - return response.Error(404, "Could not find annotation to update", err) + if canSave, err := canSaveByDashboardID(c, annotation.DashboardId); err != nil || !canSave { + return dashboardGuardianResponse(err) } existing := annotations.Item{ OrgId: c.OrgId, UserId: c.UserId, Id: annotationID, - Epoch: items[0].Time, - EpochEnd: items[0].TimeEnd, - Text: items[0].Text, - Tags: items[0].Tags, + Epoch: annotation.Time, + EpochEnd: annotation.TimeEnd, + Text: annotation.Text, + Tags: annotation.Tags, } if cmd.Tags != nil { @@ -271,16 +275,22 @@ func DeleteAnnotations(c *models.ReqContext) response.Response { } func DeleteAnnotationByID(c *models.ReqContext) response.Response { - repo := annotations.GetRepository() annotationID, err := strconv.ParseInt(web.Params(c.Req)[":annotationId"], 10, 64) if err != nil { return response.Error(http.StatusBadRequest, "annotationId is invalid", err) } - if resp := canSave(c, repo, annotationID); resp != nil { + repo := annotations.GetRepository() + + annotation, resp := findAnnotationByID(repo, annotationID, c.OrgId) + if resp != nil { return resp } + if canSave, err := canSaveByDashboardID(c, annotation.DashboardId); err != nil || !canSave { + return dashboardGuardianResponse(err) + } + err = repo.Delete(&annotations.DeleteParams{ OrgId: c.OrgId, Id: annotationID, @@ -307,19 +317,18 @@ func canSaveByDashboardID(c *models.ReqContext, dashboardID int64) (bool, error) return true, nil } -func canSave(c *models.ReqContext, repo annotations.Repository, annotationID int64) response.Response { - items, err := repo.Find(&annotations.ItemQuery{AnnotationId: annotationID, OrgId: c.OrgId}) - if err != nil || len(items) == 0 { - return response.Error(500, "Could not find annotation to update", err) +func findAnnotationByID(repo annotations.Repository, annotationID int64, orgID int64) (*annotations.ItemDTO, response.Response) { + items, err := repo.Find(&annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgID}) + + if err != nil { + return nil, response.Error(500, "Failed to find annotation", err) } - dashboardID := items[0].DashboardId - - if canSave, err := canSaveByDashboardID(c, dashboardID); err != nil || !canSave { - return dashboardGuardianResponse(err) + if len(items) == 0 { + return nil, response.Error(404, "Annotation not found", nil) } - return nil + return items[0], nil } func GetAnnotationTags(c *models.ReqContext) response.Response { diff --git a/pkg/api/annotations_test.go b/pkg/api/annotations_test.go index 365253d4016..b1dba8d0aa7 100644 --- a/pkg/api/annotations_test.go +++ b/pkg/api/annotations_test.go @@ -3,6 +3,8 @@ package api import ( "context" "fmt" + "io" + "net/http" "testing" "github.com/grafana/grafana/pkg/api/dtos" @@ -10,9 +12,11 @@ import ( "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/annotations" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAnnotationsAPIEndpoint(t *testing.T) { @@ -127,50 +131,30 @@ func TestAnnotationsAPIEndpoint(t *testing.T) { PanelId: 1, } - viewerRole := models.ROLE_VIEWER - editorRole := models.ROLE_EDITOR - - aclMockResp := []*models.DashboardAclInfoDTO{ - {Role: &viewerRole, Permission: models.PERMISSION_VIEW}, - {Role: &editorRole, Permission: models.PERMISSION_EDIT}, - } - - setUp := func() { - bus.AddHandler("test", func(ctx context.Context, query *models.GetDashboardAclInfoListQuery) error { - query.Result = aclMockResp - return nil - }) - - bus.AddHandler("test", func(ctx context.Context, query *models.GetTeamsByUserQuery) error { - query.Result = []*models.TeamDTO{} - return nil - }) - } - t.Run("When user is an Org Viewer", func(t *testing.T) { role := models.ROLE_VIEWER t.Run("Should not be allowed to save an annotation", func(t *testing.T) { postAnnotationScenario(t, "When calling POST on", "/api/annotations", "/api/annotations", role, cmd, func(sc *scenarioContext) { - setUp() + setUpACL() sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 403, sc.resp.Code) }) putAnnotationScenario(t, "When calling PUT on", "/api/annotations/1", "/api/annotations/:annotationId", role, updateCmd, func(sc *scenarioContext) { - setUp() + setUpACL() sc.fakeReqWithParams("PUT", sc.url, map[string]string{}).exec() assert.Equal(t, 403, sc.resp.Code) }) patchAnnotationScenario(t, "When calling PATCH on", "/api/annotations/1", "/api/annotations/:annotationId", role, patchCmd, func(sc *scenarioContext) { - setUp() + setUpACL() sc.fakeReqWithParams("PATCH", sc.url, map[string]string{}).exec() assert.Equal(t, 403, sc.resp.Code) }) mock := mockstore.NewSQLStoreMock() loggedInUserScenarioWithRole(t, "When calling DELETE on", "DELETE", "/api/annotations/1", "/api/annotations/:annotationId", role, func(sc *scenarioContext) { - setUp() + setUpACL() fakeAnnoRepo = &fakeAnnotationsRepo{} annotations.SetRepository(fakeAnnoRepo) sc.handlerFunc = DeleteAnnotationByID @@ -184,26 +168,26 @@ func TestAnnotationsAPIEndpoint(t *testing.T) { role := models.ROLE_EDITOR t.Run("Should be able to save an annotation", func(t *testing.T) { postAnnotationScenario(t, "When calling POST on", "/api/annotations", "/api/annotations", role, cmd, func(sc *scenarioContext) { - setUp() + setUpACL() sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 200, sc.resp.Code) }) putAnnotationScenario(t, "When calling PUT on", "/api/annotations/1", "/api/annotations/:annotationId", role, updateCmd, func(sc *scenarioContext) { - setUp() + setUpACL() sc.fakeReqWithParams("PUT", sc.url, map[string]string{}).exec() assert.Equal(t, 200, sc.resp.Code) }) patchAnnotationScenario(t, "When calling PATCH on", "/api/annotations/1", "/api/annotations/:annotationId", role, patchCmd, func(sc *scenarioContext) { - setUp() + setUpACL() sc.fakeReqWithParams("PATCH", sc.url, map[string]string{}).exec() assert.Equal(t, 200, sc.resp.Code) }) mock := mockstore.NewSQLStoreMock() loggedInUserScenarioWithRole(t, "When calling DELETE on", "DELETE", "/api/annotations/1", "/api/annotations/:annotationId", role, func(sc *scenarioContext) { - setUp() + setUpACL() fakeAnnoRepo = &fakeAnnotationsRepo{} annotations.SetRepository(fakeAnnoRepo) sc.handlerFunc = DeleteAnnotationByID @@ -217,26 +201,26 @@ func TestAnnotationsAPIEndpoint(t *testing.T) { role := models.ROLE_ADMIN t.Run("Should be able to do anything", func(t *testing.T) { postAnnotationScenario(t, "When calling POST on", "/api/annotations", "/api/annotations", role, cmd, func(sc *scenarioContext) { - setUp() + setUpACL() sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 200, sc.resp.Code) }) putAnnotationScenario(t, "When calling PUT on", "/api/annotations/1", "/api/annotations/:annotationId", role, updateCmd, func(sc *scenarioContext) { - setUp() + setUpACL() sc.fakeReqWithParams("PUT", sc.url, map[string]string{}).exec() assert.Equal(t, 200, sc.resp.Code) }) patchAnnotationScenario(t, "When calling PATCH on", "/api/annotations/1", "/api/annotations/:annotationId", role, patchCmd, func(sc *scenarioContext) { - setUp() + setUpACL() sc.fakeReqWithParams("PATCH", sc.url, map[string]string{}).exec() assert.Equal(t, 200, sc.resp.Code) }) deleteAnnotationsScenario(t, "When calling POST on", "/api/annotations/mass-delete", "/api/annotations/mass-delete", role, deleteCmd, func(sc *scenarioContext) { - setUp() + setUpACL() sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 200, sc.resp.Code) }) @@ -373,3 +357,87 @@ func deleteAnnotationsScenario(t *testing.T, desc string, url string, routePatte fn(sc) }) } + +func TestAPI_Annotations_AccessControl(t *testing.T) { + sc := setupHTTPServer(t, true, true) + setInitCtxSignedInEditor(sc.initCtx) + _, err := sc.db.CreateOrgWithMember("TestOrg", testUserID) + require.NoError(t, err) + + type args struct { + permissions []*accesscontrol.Permission + url string + body io.Reader + method string + } + + tests := []struct { + name string + args args + want int + }{ + { + name: "AccessControl getting annotations with correct permissions is allowed", + args: args{ + permissions: []*accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsRead, Scope: accesscontrol.ScopeAnnotationsAll}}, + url: "/api/annotations", + method: http.MethodGet, + }, + want: 200, + }, + { + name: "AccessControl getting annotations without permissions is forbidden", + args: args{ + permissions: []*accesscontrol.Permission{}, + url: "/api/annotations", + method: http.MethodGet, + }, + want: 403, + }, + { + name: "AccessControl getting tags for annotations with correct permissions is allowed", + args: args{ + permissions: []*accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsTagsRead, Scope: accesscontrol.ScopeAnnotationsTagsAll}}, + url: "/api/annotations/tags", + method: http.MethodGet, + }, + want: 200, + }, + { + name: "AccessControl getting tags for annotations without correct permissions is forbidden", + args: args{ + permissions: []*accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsTagsRead}}, + url: "/api/annotations/tags", + method: http.MethodGet, + }, + want: 403, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + setAccessControlPermissions(sc.acmock, tt.args.permissions, sc.initCtx.OrgId) + r := callAPI(sc.server, tt.args.method, tt.args.url, tt.args.body, t) + assert.Equalf(t, tt.want, r.Code, "Annotations API(%v)", tt.args.url) + }) + } +} + +func setUpACL() { + viewerRole := models.ROLE_VIEWER + editorRole := models.ROLE_EDITOR + + aclMockResp := []*models.DashboardAclInfoDTO{ + {Role: &viewerRole, Permission: models.PERMISSION_VIEW}, + {Role: &editorRole, Permission: models.PERMISSION_EDIT}, + } + + bus.AddHandler("test", func(ctx context.Context, query *models.GetDashboardAclInfoListQuery) error { + query.Result = aclMockResp + return nil + }) + + bus.AddHandler("test", func(ctx context.Context, query *models.GetTeamsByUserQuery) error { + query.Result = []*models.TeamDTO{} + return nil + }) +} diff --git a/pkg/api/api.go b/pkg/api/api.go index dba7d43ad0e..6da0aa3488c 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -414,7 +414,7 @@ func (hs *HTTPServer) registerRoutes() { orgRoute.Get("/lookup", routing.Wrap(hs.GetAlertNotificationLookup)) }) - apiRoute.Get("/annotations", routing.Wrap(GetAnnotations)) + apiRoute.Get("/annotations", authorize(reqSignedIn, ac.EvalPermission(ac.ActionAnnotationsRead, ac.ScopeAnnotationsAll)), routing.Wrap(GetAnnotations)) apiRoute.Post("/annotations/mass-delete", reqOrgAdmin, routing.Wrap(DeleteAnnotations)) apiRoute.Group("/annotations", func(annotationsRoute routing.RouteRegister) { @@ -423,7 +423,7 @@ func (hs *HTTPServer) registerRoutes() { annotationsRoute.Put("/:annotationId", routing.Wrap(UpdateAnnotation)) annotationsRoute.Patch("/:annotationId", routing.Wrap(PatchAnnotation)) annotationsRoute.Post("/graphite", reqEditorRole, routing.Wrap(PostGraphiteAnnotation)) - annotationsRoute.Get("/tags", routing.Wrap(GetAnnotationTags)) + annotationsRoute.Get("/tags", authorize(reqSignedIn, ac.EvalPermission(ac.ActionAnnotationsTagsRead, ac.ScopeAnnotationsTagsAll)), routing.Wrap(GetAnnotationTags)) }) apiRoute.Post("/frontend-metrics", routing.Wrap(hs.PostFrontendMetrics)) diff --git a/pkg/api/roles.go b/pkg/api/roles.go index f168731dcc4..8ec62c68ea7 100644 --- a/pkg/api/roles.go +++ b/pkg/api/roles.go @@ -245,10 +245,25 @@ func (hs *HTTPServer) declareFixedRoles() error { Grants: []string{string(models.ROLE_ADMIN)}, } + annotationsReaderRole := accesscontrol.RoleRegistration{ + Role: accesscontrol.RoleDTO{ + Name: "fixed:annotations:reader", + DisplayName: "Annotation reader", + Description: "Read annotations and tags", + Group: "Annotations", + Version: 1, + Permissions: []accesscontrol.Permission{ + {Action: accesscontrol.ActionAnnotationsRead, Scope: accesscontrol.ScopeAnnotationsAll}, + {Action: accesscontrol.ActionAnnotationsTagsRead, Scope: accesscontrol.ScopeAnnotationsTagsAll}, + }, + }, + Grants: []string{string(models.ROLE_VIEWER)}, + } + return hs.AccessControl.DeclareFixedRoles( provisioningWriterRole, datasourcesReaderRole, datasourcesWriterRole, datasourcesIdReaderRole, - datasourcesCompatibilityReaderRole, orgReaderRole, orgWriterRole, - orgMaintainerRole, teamsCreatorRole, teamsWriterRole, datasourcesExplorerRole, + datasourcesCompatibilityReaderRole, orgReaderRole, orgWriterRole, orgMaintainerRole, teamsCreatorRole, + teamsWriterRole, datasourcesExplorerRole, annotationsReaderRole, ) } diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index e3e71970519..99236f7ee36 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -321,6 +321,14 @@ const ( // Team related scopes ScopeTeamsAll = "teams:*" + + // Annotations related actions + ActionAnnotationsRead = "annotations:read" + + ActionAnnotationsTagsRead = "annotations.tags:read" + + ScopeAnnotationsAll = "annotations:*" + ScopeAnnotationsTagsAll = "annotations:tags:*" ) var (