Annotations: Remove dashboard permission checks for annotations (#78352)

remove checks for access to dashboard if FlagAnnotationPermissionUpdate is enabled
This commit is contained in:
Ieva 2023-11-23 10:47:37 +00:00 committed by GitHub
parent 05070385cd
commit 778841cabe
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 241 additions and 20 deletions

View File

@ -127,7 +127,13 @@ func (hs *HTTPServer) PostAnnotation(c *contextmodel.ReqContext) response.Respon
}
if canSave, err := hs.canCreateAnnotation(c, cmd.DashboardId); err != nil || !canSave {
return dashboardGuardianResponse(err)
if !hs.Features.IsEnabled(c.Req.Context(), featuremgmt.FlagAnnotationPermissionUpdate) {
return dashboardGuardianResponse(err)
} else if err != nil {
return response.Error(http.StatusInternalServerError, "Error while checking annotation permissions", err)
} else {
return response.Error(http.StatusForbidden, "Access denied to save the annotation", nil)
}
}
if cmd.Text == "" {
@ -273,8 +279,10 @@ func (hs *HTTPServer) UpdateAnnotation(c *contextmodel.ReqContext) response.Resp
return resp
}
if canSave, err := hs.canSaveAnnotation(c, annotation); err != nil || !canSave {
return dashboardGuardianResponse(err)
if !hs.Features.IsEnabled(c.Req.Context(), featuremgmt.FlagAnnotationPermissionUpdate) {
if canSave, err := hs.canSaveAnnotation(c, annotation); err != nil || !canSave {
return dashboardGuardianResponse(err)
}
}
userID, err := identity.UserIdentifier(c.SignedInUser.GetNamespacedID())
@ -334,8 +342,10 @@ func (hs *HTTPServer) PatchAnnotation(c *contextmodel.ReqContext) response.Respo
return resp
}
if canSave, err := hs.canSaveAnnotation(c, annotation); err != nil || !canSave {
return dashboardGuardianResponse(err)
if !hs.Features.IsEnabled(c.Req.Context(), featuremgmt.FlagAnnotationPermissionUpdate) {
if canSave, err := hs.canSaveAnnotation(c, annotation); err != nil || !canSave {
return dashboardGuardianResponse(err)
}
}
userID, err := identity.UserIdentifier(c.SignedInUser.GetNamespacedID())
@ -437,7 +447,13 @@ func (hs *HTTPServer) MassDeleteAnnotations(c *contextmodel.ReqContext) response
canSave, err := hs.canMassDeleteAnnotations(c, dashboardId)
if err != nil || !canSave {
return dashboardGuardianResponse(err)
if !hs.Features.IsEnabled(c.Req.Context(), featuremgmt.FlagAnnotationPermissionUpdate) {
return dashboardGuardianResponse(err)
} else if err != nil {
return response.Error(http.StatusInternalServerError, "Error while checking annotation permissions", err)
} else {
return response.Error(http.StatusForbidden, "Access denied to mass delete annotations", nil)
}
}
err = hs.annotationsRepo.Delete(c.Req.Context(), deleteParams)
@ -492,13 +508,15 @@ func (hs *HTTPServer) DeleteAnnotationByID(c *contextmodel.ReqContext) response.
return response.Error(http.StatusBadRequest, "annotationId is invalid", err)
}
annotation, resp := findAnnotationByID(c.Req.Context(), hs.annotationsRepo, annotationID, c.SignedInUser)
if resp != nil {
return resp
}
if !hs.Features.IsEnabled(c.Req.Context(), featuremgmt.FlagAnnotationPermissionUpdate) {
annotation, resp := findAnnotationByID(c.Req.Context(), hs.annotationsRepo, annotationID, c.SignedInUser)
if resp != nil {
return resp
}
if canSave, err := hs.canSaveAnnotation(c, annotation); err != nil || !canSave {
return dashboardGuardianResponse(err)
if canSave, err := hs.canSaveAnnotation(c, annotation); err != nil || !canSave {
return dashboardGuardianResponse(err)
}
}
err = hs.annotationsRepo.Delete(c.Req.Context(), &annotations.DeleteParams{
@ -658,6 +676,16 @@ func AnnotationTypeScopeResolver(annotationsRepo annotations.Repository, feature
}
func (hs *HTTPServer) canCreateAnnotation(c *contextmodel.ReqContext, dashboardId int64) (bool, error) {
if hs.Features.IsEnabled(c.Req.Context(), featuremgmt.FlagAnnotationPermissionUpdate) {
if dashboardId != 0 {
evaluator := accesscontrol.EvalPermission(accesscontrol.ActionAnnotationsCreate, dashboards.ScopeDashboardsProvider.GetResourceScope(strconv.FormatInt(dashboardId, 10)))
return hs.AccessControl.Evaluate(c.Req.Context(), c.SignedInUser, evaluator)
} else { // organization annotations
evaluator := accesscontrol.EvalPermission(accesscontrol.ActionAnnotationsCreate, accesscontrol.ScopeAnnotationsTypeOrganization)
return hs.AccessControl.Evaluate(c.Req.Context(), c.SignedInUser, evaluator)
}
}
if dashboardId != 0 {
evaluator := accesscontrol.EvalPermission(accesscontrol.ActionAnnotationsCreate, accesscontrol.ScopeAnnotationsTypeDashboard)
if canSave, err := hs.AccessControl.Evaluate(c.Req.Context(), c.SignedInUser, evaluator); err != nil || !canSave {
@ -672,6 +700,16 @@ func (hs *HTTPServer) canCreateAnnotation(c *contextmodel.ReqContext, dashboardI
}
func (hs *HTTPServer) canMassDeleteAnnotations(c *contextmodel.ReqContext, dashboardID int64) (bool, error) {
if hs.Features.IsEnabled(c.Req.Context(), featuremgmt.FlagAnnotationPermissionUpdate) {
if dashboardID == 0 {
evaluator := accesscontrol.EvalPermission(accesscontrol.ActionAnnotationsDelete, accesscontrol.ScopeAnnotationsTypeOrganization)
return hs.AccessControl.Evaluate(c.Req.Context(), c.SignedInUser, evaluator)
} else {
evaluator := accesscontrol.EvalPermission(accesscontrol.ActionAnnotationsDelete, dashboards.ScopeDashboardsProvider.GetResourceScope(strconv.FormatInt(dashboardID, 10)))
return hs.AccessControl.Evaluate(c.Req.Context(), c.SignedInUser, evaluator)
}
}
if dashboardID == 0 {
evaluator := accesscontrol.EvalPermission(accesscontrol.ActionAnnotationsDelete, accesscontrol.ScopeAnnotationsTypeOrganization)
return hs.AccessControl.Evaluate(c.Req.Context(), c.SignedInUser, evaluator)

View File

@ -8,6 +8,7 @@ import (
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"github.com/grafana/grafana/pkg/services/accesscontrol"
@ -16,6 +17,7 @@ import (
"github.com/grafana/grafana/pkg/services/annotations/annotationstest"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/folder"
"github.com/grafana/grafana/pkg/services/folder/foldertest"
"github.com/grafana/grafana/pkg/services/guardian"
"github.com/grafana/grafana/pkg/setting"
@ -23,12 +25,16 @@ import (
)
func TestAPI_Annotations(t *testing.T) {
dashUID := "test-dash"
folderUID := "test-folder"
type testCase struct {
desc string
path string
method string
body string
expectedCode int
featureFlags []any
permissions []accesscontrol.Permission
}
@ -61,6 +67,30 @@ func TestAPI_Annotations(t *testing.T) {
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{},
},
{
desc: "should be able to fetch dashboard annotation by id with correct dashboard scope with annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodGet,
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsRead, Scope: dashboards.ScopeDashboardsProvider.GetResourceScopeUID(dashUID)}},
},
{
desc: "should be able to fetch dashboard annotation by id with correct folder scope with annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodGet,
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsRead, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID(folderUID)}},
},
{
desc: "should not be able to fetch dashboard annotation by id with the old dashboard scope when annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodGet,
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsRead, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}},
},
{
desc: "should be able to fetch annotation tags with correct permission",
path: "/api/annotations/tags",
@ -89,6 +119,30 @@ func TestAPI_Annotations(t *testing.T) {
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{},
},
{
desc: "should be able to update dashboard annotation with correct dashboard scope with annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodPut,
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsWrite, Scope: dashboards.ScopeDashboardsProvider.GetResourceScopeUID(dashUID)}},
},
{
desc: "should be able to update dashboard annotation with correct folder scope with annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodPut,
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsWrite, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID(folderUID)}},
},
{
desc: "should not be able to update dashboard annotation with the old dashboard scope when annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodPut,
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsWrite, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}},
},
{
desc: "should be able to update organization annotation with correct permission",
path: "/api/annotations/1",
@ -117,6 +171,30 @@ func TestAPI_Annotations(t *testing.T) {
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{},
},
{
desc: "should be able to patch dashboard annotation with correct dashboard scope with annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodPatch,
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsWrite, Scope: dashboards.ScopeDashboardsProvider.GetResourceScopeUID(dashUID)}},
},
{
desc: "should be able to patch dashboard annotation with correct folder scope with annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodPatch,
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsWrite, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID(folderUID)}},
},
{
desc: "should not be able to patch dashboard annotation with the old dashboard scope when annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodPatch,
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsWrite, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}},
},
{
desc: "should be able to patch organization annotation with correct permission",
path: "/api/annotations/1",
@ -147,6 +225,33 @@ func TestAPI_Annotations(t *testing.T) {
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsCreate, Scope: accesscontrol.ScopeAnnotationsTypeOrganization}},
},
{
desc: "should be able to create dashboard annotation with correct dashboard scope with annotationPermissionUpdate enabled",
path: "/api/annotations",
method: http.MethodPost,
body: "{\"dashboardId\": 2,\"text\": \"test\"}",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsCreate, Scope: dashboards.ScopeDashboardsProvider.GetResourceScopeUID(dashUID)}},
},
{
desc: "should be able to create dashboard annotation with correct folder scope with annotationPermissionUpdate enabled",
path: "/api/annotations",
method: http.MethodPost,
body: "{\"dashboardId\": 2,\"text\": \"test\"}",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsCreate, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID(folderUID)}},
},
{
desc: "should not be able to create dashboard annotation with the old dashboard scope when annotationPermissionUpdate enabled",
path: "/api/annotations",
method: http.MethodPost,
body: "{\"dashboardId\": 2,\"text\": \"test\"}",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsCreate, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}},
},
{
desc: "should be able to create organization annotation with correct permission",
path: "/api/annotations",
@ -177,6 +282,30 @@ func TestAPI_Annotations(t *testing.T) {
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsDelete, Scope: accesscontrol.ScopeAnnotationsTypeOrganization}},
},
{
desc: "should be able to delete dashboard annotation with correct dashboard scope with annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodDelete,
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsDelete, Scope: dashboards.ScopeDashboardsProvider.GetResourceScopeUID(dashUID)}},
},
{
desc: "should be able to delete dashboard annotation with correct folder scope with annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodDelete,
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsDelete, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID(folderUID)}},
},
{
desc: "should not be able to delete dashboard annotation with the old dashboard scope when annotationPermissionUpdate enabled",
path: "/api/annotations/2",
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
method: http.MethodDelete,
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsDelete, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}},
},
{
desc: "should be able to delete organization annotation with correct permission",
path: "/api/annotations/1",
@ -222,22 +351,59 @@ func TestAPI_Annotations(t *testing.T) {
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsDelete, Scope: accesscontrol.ScopeAnnotationsTypeOrganization}},
},
{
desc: "should be able to mass delete dashboard annotation with correct dashboard scope with annotationPermissionUpdate enabled",
path: "/api/annotations/mass-delete",
body: "{\"dashboardId\": 2, \"panelId\": 1}",
method: http.MethodPost,
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsDelete, Scope: dashboards.ScopeDashboardsProvider.GetResourceScopeUID(dashUID)}},
},
{
desc: "should be able to mass delete dashboard annotation with correct folder scope with annotationPermissionUpdate enabled",
path: "/api/annotations/mass-delete",
body: "{\"dashboardId\": 2, \"panelId\": 1}",
method: http.MethodPost,
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
expectedCode: http.StatusOK,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsDelete, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID(folderUID)}},
},
{
desc: "should not be able to mass delete dashboard annotation with the old dashboard scope when annotationPermissionUpdate enabled",
path: "/api/annotations/mass-delete",
body: "{\"dashboardId\": 2, \"panelId\": 1}",
method: http.MethodPost,
featureFlags: []any{featuremgmt.FlagAnnotationPermissionUpdate},
expectedCode: http.StatusForbidden,
permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsDelete, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}},
},
}
for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
setUpRBACGuardian(t)
// Don't need access to dashboards if annotationPermissionUpdate is enabled
if len(tt.featureFlags) == 0 {
setUpRBACGuardian(t)
}
server := SetupAPITestServer(t, func(hs *HTTPServer) {
hs.Cfg = setting.NewCfg()
repo := annotationstest.NewFakeAnnotationsRepo()
_ = repo.Save(context.Background(), &annotations.Item{ID: 1, DashboardID: 0})
_ = repo.Save(context.Background(), &annotations.Item{ID: 2, DashboardID: 1})
hs.annotationsRepo = repo
hs.Features = featuremgmt.WithFeatures(tt.featureFlags...)
dashService := &dashboards.FakeDashboardService{}
dashService.On("GetDashboard", mock.Anything, mock.Anything).Return(&dashboards.Dashboard{UID: dashUID, FolderUID: folderUID, FolderID: 1}, nil)
folderService := &foldertest.FakeService{}
folderService.ExpectedFolder = &folder.Folder{UID: folderUID, ID: 1}
folderDB := &foldertest.FakeFolderStore{}
folderDB.On("GetFolderByID", mock.Anything, mock.Anything, mock.Anything).Return(&folder.Folder{UID: folderUID, ID: 1}, nil)
hs.DashboardService = dashService
hs.folderService = folderService
hs.AccessControl = acimpl.ProvideAccessControl(hs.Cfg)
features := featuremgmt.WithFeatures()
dashSvc := &dashboards.FakeDashboardService{}
folderSvc := &foldertest.FakeService{}
hs.AccessControl.RegisterScopeAttributeResolver(AnnotationTypeScopeResolver(hs.annotationsRepo, features, dashSvc, folderSvc))
hs.AccessControl.RegisterScopeAttributeResolver(AnnotationTypeScopeResolver(hs.annotationsRepo, hs.Features, dashService, folderService))
hs.AccessControl.RegisterScopeAttributeResolver(dashboards.NewDashboardIDScopeResolver(folderDB, dashService, folderService))
})
var body io.Reader
if tt.body != "" {
@ -252,6 +418,7 @@ func TestAPI_Annotations(t *testing.T) {
})
}
}
func TestService_AnnotationTypeScopeResolver(t *testing.T) {
rootDashUID := "root-dashboard"
folderDashUID := "folder-dashboard"

View File

@ -144,7 +144,11 @@ func (hs *HTTPServer) GetDashboard(c *contextmodel.ReqContext) response.Response
}
annotationPermissions := &dtos.AnnotationPermission{}
hs.getAnnotationPermissionsByScope(c, &annotationPermissions.Dashboard, accesscontrol.ScopeAnnotationsTypeDashboard)
if hs.Features.IsEnabled(c.Req.Context(), featuremgmt.FlagAnnotationPermissionUpdate) {
hs.getAnnotationPermissionsByScope(c, &annotationPermissions.Dashboard, dashboards.ScopeDashboardsProvider.GetResourceScopeUID(dash.UID))
} else {
hs.getAnnotationPermissionsByScope(c, &annotationPermissions.Dashboard, accesscontrol.ScopeAnnotationsTypeDashboard)
}
hs.getAnnotationPermissionsByScope(c, &annotationPermissions.Organization, accesscontrol.ScopeAnnotationsTypeOrganization)
meta := dtos.DashboardMeta{

View File

@ -1181,6 +1181,10 @@ export class DashboardModel implements TimeModel {
} else {
canEdit = !!this.meta.annotationsPermissions?.dashboard.canEdit;
}
if (config.featureToggles.annotationPermissionUpdate) {
return canEdit;
}
return this.canEditDashboard() && canEdit;
}
@ -1193,18 +1197,26 @@ export class DashboardModel implements TimeModel {
} else {
canDelete = !!this.meta.annotationsPermissions?.dashboard.canDelete;
}
if (config.featureToggles.annotationPermissionUpdate) {
return canDelete;
}
return canDelete && this.canEditDashboard();
}
canAddAnnotations() {
// When the builtin annotations are disabled, we should not add any in the UI
const found = this.annotations.list.find((item) => item.builtIn === 1);
if (found?.enable === false || !this.canEditDashboard()) {
if (found?.enable === false) {
return false;
}
// If RBAC is enabled there are additional conditions to check.
return Boolean(this.meta.annotationsPermissions?.dashboard.canAdd);
if (config.featureToggles.annotationPermissionUpdate) {
return Boolean(this.meta.annotationsPermissions?.dashboard.canAdd);
}
return Boolean(this.meta.annotationsPermissions?.dashboard.canAdd) && this.canEditDashboard();
}
canEditDashboard() {