public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

RBAC: Remove accessControlOnCall feature toggle (#101222)

Browse Source 
* RBAC: Remove accessControlOnCall feature toggle

* Leave the other one in place

* Tests

* frontend

* Readd empty ft to frontend test

* Remove legacy RBAC check

* Fix test

* no need for context

* Remove unused variable

* Remove unecessary param

* remove unecessary param from tests

* More tests :D
This commit is contained in:
Gabriel MABILLE
2025-02-25 13:44:40 +01:00
committed by GitHub
parent 1a65154e74
commit f3433fd472
23 changed files with 88 additions and 175 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/accesscontrol/acimpl/service.go
+2 -7
View File
@@ -68,7 +68,7 @@ func ProvideService(
lock,
)
api.NewAccessControlAPI(routeRegister, accessControl, service, userService, features).RegisterAPIEndpoints()
api.NewAccessControlAPI(routeRegister, accessControl, service, userService).RegisterAPIEndpoints()
if err := accesscontrol.DeclareFixedRoles(service, cfg); err != nil {
return nil, err
}
@@ -472,14 +472,9 @@ func (s *Service) RegisterFixedRoles(ctx context.Context) error {
// DeclarePluginRoles allow the caller to declare, to the service, plugin roles and their assignments
// to organization roles ("Viewer", "Editor", "Admin") or "Grafana Admin"
func (s *Service) DeclarePluginRoles(ctx context.Context, ID, name string, regs []plugins.RoleRegistration) error {
ctx, span := tracer.Start(ctx, "accesscontrol.acimpl.DeclarePluginRoles")
_, span := tracer.Start(ctx, "accesscontrol.acimpl.DeclarePluginRoles")
defer span.End()
// Protect behind feature toggle
if !s.features.IsEnabled(ctx, featuremgmt.FlagAccessControlOnCall) {
return nil
}
acRegs := pluginutils.ToRegistrations(ID, name, regs)
for _, r := range acRegs {
if err := pluginutils.ValidatePluginRole(ID, r.Role); err != nil {
pkg/services/accesscontrol/acimpl/service_test.go
-1
View File
@@ -253,7 +253,6 @@ func TestService_DeclarePluginRoles(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ac := setupTestEnv(t)
ac.features = featuremgmt.WithFeatures(featuremgmt.FlagAccessControlOnCall)
// Reset the registations
ac.registrations = accesscontrol.RegistrationList{}
pkg/services/accesscontrol/api/api.go
+2 -8
View File
@@ -17,20 +17,17 @@ import (
"github.com/grafana/grafana/pkg/middleware/requestmeta"
ac "github.com/grafana/grafana/pkg/services/accesscontrol"
contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/user"
)
var tracer = otel.Tracer("github.com/grafana/grafana/pkg/services/accesscontrol/api")
func NewAccessControlAPI(router routing.RouteRegister, accesscontrol ac.AccessControl, service ac.Service,
userSvc user.Service, features featuremgmt.FeatureToggles) *AccessControlAPI {
func NewAccessControlAPI(router routing.RouteRegister, accesscontrol ac.AccessControl, service ac.Service, userSvc user.Service) *AccessControlAPI {
return &AccessControlAPI{
RouteRegister: router,
Service: service,
userSvc: userSvc,
AccessControl: accesscontrol,
features: features,
}
}
@@ -39,7 +36,6 @@ type AccessControlAPI struct {
AccessControl ac.AccessControl
RouteRegister routing.RouteRegister
userSvc user.Service
features featuremgmt.FeatureToggles
}
func (api *AccessControlAPI) RegisterAPIEndpoints() {
@@ -48,9 +44,7 @@ func (api *AccessControlAPI) RegisterAPIEndpoints() {
api.RouteRegister.Group("/api/access-control", func(rr routing.RouteRegister) {
rr.Get("/user/actions", middleware.ReqSignedIn, routing.Wrap(api.getUserActions))
rr.Get("/user/permissions", middleware.ReqSignedIn, routing.Wrap(api.getUserPermissions))
if api.features.IsEnabledGlobally(featuremgmt.FlagAccessControlOnCall) {
rr.Get("/users/permissions/search", authorize(ac.EvalPermission(ac.ActionUsersPermissionsRead)), routing.Wrap(api.searchUsersPermissions))
}
rr.Get("/users/permissions/search", authorize(ac.EvalPermission(ac.ActionUsersPermissionsRead)), routing.Wrap(api.searchUsersPermissions))
}, requestmeta.SetOwner(requestmeta.TeamAuth))
}
pkg/services/accesscontrol/api/api_test.go
+3 -4
View File
@@ -12,7 +12,6 @@ import (
ac "github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
"github.com/grafana/grafana/pkg/services/datasources"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/services/user/usertest"
"github.com/grafana/grafana/pkg/util"
@@ -42,7 +41,7 @@ func TestAPI_getUserActions(t *testing.T) {
for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
acSvc := actest.FakeService{ExpectedPermissions: tt.permissions}
api := NewAccessControlAPI(routing.NewRouteRegister(), actest.FakeAccessControl{}, acSvc, &usertest.FakeUserService{}, featuremgmt.WithFeatures())
api := NewAccessControlAPI(routing.NewRouteRegister(), actest.FakeAccessControl{}, acSvc, &usertest.FakeUserService{})
api.RegisterAPIEndpoints()
server := webtest.NewServer(t, api.RouteRegister)
@@ -95,7 +94,7 @@ func TestAPI_getUserPermissions(t *testing.T) {
for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
acSvc := actest.FakeService{ExpectedPermissions: tt.permissions}
api := NewAccessControlAPI(routing.NewRouteRegister(), actest.FakeAccessControl{}, acSvc, &usertest.FakeUserService{}, featuremgmt.WithFeatures())
api := NewAccessControlAPI(routing.NewRouteRegister(), actest.FakeAccessControl{}, acSvc, &usertest.FakeUserService{})
api.RegisterAPIEndpoints()
server := webtest.NewServer(t, api.RouteRegister)
@@ -192,7 +191,7 @@ func TestAccessControlAPI_searchUsersPermissions(t *testing.T) {
mockUserSvc := usertest.NewMockService(t)
mockUserSvc.On("GetByUID", mock.Anything, &user.GetUserByUIDQuery{UID: "user_2_uid"}).Return(&user.User{ID: 2}, nil).Maybe()
mockUserSvc.On("GetByUID", mock.Anything, &user.GetUserByUIDQuery{UID: "non_existent_uid"}).Return(nil, user.ErrUserNotFound).Maybe()
api := NewAccessControlAPI(routing.NewRouteRegister(), accessControl, acSvc, mockUserSvc, featuremgmt.WithFeatures(featuremgmt.FlagAccessControlOnCall))
api := NewAccessControlAPI(routing.NewRouteRegister(), accessControl, acSvc, mockUserSvc)
api.RegisterAPIEndpoints()
server := webtest.NewServer(t, api.RouteRegister)
pkg/services/accesscontrol/resourcepermissions/service.go
+1 -1
View File
@@ -583,7 +583,7 @@ func (a *ActionSetSvc) RegisterActionSets(ctx context.Context, pluginID string,
ctx, span := tracer.Start(ctx, "accesscontrol.resourcepermissions.RegisterActionSets")
defer span.End()
if !a.features.IsEnabled(ctx, featuremgmt.FlagAccessActionSets) || !a.features.IsEnabled(ctx, featuremgmt.FlagAccessControlOnCall) {
if !a.features.IsEnabled(ctx, featuremgmt.FlagAccessActionSets) {
return nil
}
for _, reg := range registrations {
pkg/services/accesscontrol/resourcepermissions/service_test.go
+6 -6
View File
@@ -328,7 +328,7 @@ func TestStore_RegisterActionSet(t *testing.T) {
tests := []actionSetTest{
{
desc: "should be able to register a plugin action set if the right feature toggles are enabled",
features: featuremgmt.WithFeatures(featuremgmt.FlagAccessActionSets, featuremgmt.FlagAccessControlOnCall),
features: featuremgmt.WithFeatures(featuremgmt.FlagAccessActionSets),
pluginID: "test-app",
pluginActions: []plugins.ActionSet{
{
@@ -345,7 +345,7 @@ func TestStore_RegisterActionSet(t *testing.T) {
},
{
desc: "should not register plugin action set if feature toggles are missing",
features: featuremgmt.WithFeatures(featuremgmt.FlagAccessControlOnCall),
features: featuremgmt.WithFeatures(),
pluginID: "test-app",
pluginActions: []plugins.ActionSet{
{
@@ -357,7 +357,7 @@ func TestStore_RegisterActionSet(t *testing.T) {
},
{
desc: "should be able to register multiple plugin action sets",
features: featuremgmt.WithFeatures(featuremgmt.FlagAccessActionSets, featuremgmt.FlagAccessControlOnCall),
features: featuremgmt.WithFeatures(featuremgmt.FlagAccessActionSets),
pluginID: "test-app",
pluginActions: []plugins.ActionSet{
{
@@ -382,7 +382,7 @@ func TestStore_RegisterActionSet(t *testing.T) {
},
{
desc: "action set actions should be added not replaced",
features: featuremgmt.WithFeatures(featuremgmt.FlagAccessActionSets, featuremgmt.FlagAccessControlOnCall),
features: featuremgmt.WithFeatures(featuremgmt.FlagAccessActionSets),
pluginID: "test-app",
pluginActions: []plugins.ActionSet{
{
@@ -425,7 +425,7 @@ func TestStore_RegisterActionSet(t *testing.T) {
},
{
desc: "should not be able to register an action that doesn't have a plugin prefix",
features: featuremgmt.WithFeatures(featuremgmt.FlagAccessActionSets, featuremgmt.FlagAccessControlOnCall),
features: featuremgmt.WithFeatures(featuremgmt.FlagAccessActionSets),
pluginID: "test-app",
pluginActions: []plugins.ActionSet{
{
@@ -441,7 +441,7 @@ func TestStore_RegisterActionSet(t *testing.T) {
},
{
desc: "should not be able to register action set that is not in the allow list",
features: featuremgmt.WithFeatures(featuremgmt.FlagAccessActionSets, featuremgmt.FlagAccessControlOnCall),
features: featuremgmt.WithFeatures(featuremgmt.FlagAccessActionSets),
pluginID: "test-app",
pluginActions: []plugins.ActionSet{
{
pkg/services/featuremgmt/registry.go
-8
View File
@@ -198,14 +198,6 @@ var (
Stage: FeatureStageExperimental,
Owner: grafanaSearchAndStorageSquad,
},
{
Name: "accessControlOnCall",
Description: "Access control primitives for OnCall",
Stage: FeatureStageGeneralAvailability,
Owner: identityAccessTeam,
HideFromAdminPage: true,
Expression: "true", // enabled by default
},
{
Name: "nestedFolders",
Description: "Enable folder nesting",
pkg/services/featuremgmt/toggles_gen.csv
-1
View File
@@ -24,7 +24,6 @@ grpcServer,preview,@grafana/search-and-storage,false,false,false
cloudWatchCrossAccountQuerying,GA,@grafana/aws-datasources,false,false,false
showDashboardValidationWarnings,experimental,@grafana/dashboards-squad,false,false,false
mysqlAnsiQuotes,experimental,@grafana/search-and-storage,false,false,false
accessControlOnCall,GA,@grafana/identity-access-team,false,false,false
nestedFolders,GA,@grafana/search-and-storage,false,false,false
alertingBacktesting,experimental,@grafana/alerting-squad,false,false,false
editPanelCSVDragAndDrop,experimental,@grafana/dataviz-squad,false,false,true
1 Name Stage Owner requiresDevMode RequiresRestart FrontendOnly
24 cloudWatchCrossAccountQuerying GA @grafana/aws-datasources false false false
25 showDashboardValidationWarnings experimental @grafana/dashboards-squad false false false
26 mysqlAnsiQuotes experimental @grafana/search-and-storage false false false
accessControlOnCall GA @grafana/identity-access-team false false false
27 nestedFolders GA @grafana/search-and-storage false false false
28 alertingBacktesting experimental @grafana/alerting-squad false false false
29 editPanelCSVDragAndDrop experimental @grafana/dataviz-squad false false true
pkg/services/featuremgmt/toggles_gen.go
-4
View File
@@ -107,10 +107,6 @@ const (
// Use double quotes to escape keyword in a MySQL query
FlagMysqlAnsiQuotes = "mysqlAnsiQuotes"
// FlagAccessControlOnCall
// Access control primitives for OnCall
FlagAccessControlOnCall = "accessControlOnCall"
// FlagNestedFolders
// Enable folder nesting
FlagNestedFolders = "nestedFolders"
pkg/services/featuremgmt/toggles_gen.json
+1
View File
@@ -52,6 +52,7 @@
"name": "accessControlOnCall",
"resourceVersion": "1726562036211",
"creationTimestamp": "2022-10-19T16:10:09Z",
"deletionTimestamp": "2025-02-24T14:40:54Z",
"annotations": {
"grafana.app/updatedTimestamp": "2024-09-17 08:33:56.211355566 +0000 UTC"
}
pkg/services/navtree/navtreeimpl/applinks.go
+2 -3
View File
@@ -268,13 +268,12 @@ func (s *ServiceImpl) addPluginToSection(c *contextmodel.ReqContext, treeRoot *n
func (s *ServiceImpl) hasAccessToInclude(c *contextmodel.ReqContext, pluginID string) func(include *plugins.Includes) bool {
hasAccess := ac.HasAccess(s.accessControl, c)
return func(include *plugins.Includes) bool {
useRBAC := s.features.IsEnabledGlobally(featuremgmt.FlagAccessControlOnCall) && include.RequiresRBACAction()
if useRBAC && !hasAccess(pluginaccesscontrol.GetPluginRouteEvaluator(pluginID, include.Action)) {
if include.RequiresRBACAction() && !hasAccess(pluginaccesscontrol.GetPluginRouteEvaluator(pluginID, include.Action)) {
s.log.Debug("plugin include is covered by RBAC, user doesn't have access",
"plugin", pluginID,
"include", include.Name)
return false
} else if !useRBAC && !c.HasUserRole(include.Role) {
} else if !include.RequiresRBACAction() && !c.HasUserRole(include.Role) {
return false
}
return true
pkg/services/navtree/navtreeimpl/applinks_test.go
+57 -104
View File
@@ -450,114 +450,67 @@ func TestAddAppLinksAccessControl(t *testing.T) {
},
}
t.Run("Without plugin RBAC - Enforce role", func(t *testing.T) {
t.Run("Should not add app links when the user cannot access app plugins", func(t *testing.T) {
treeRoot := navtree.NavTreeRoot{}
user.Permissions = map[int64]map[string][]string{}
user.OrgRole = identity.RoleAdmin
t.Run("Should not see any includes with no app access", func(t *testing.T) {
treeRoot := navtree.NavTreeRoot{}
user.Permissions = map[int64]map[string][]string{
1: {pluginaccesscontrol.ActionAppAccess: []string{"plugins:id:not-the-test-app1"}},
}
user.OrgRole = identity.RoleNone
service.features = featuremgmt.WithFeatures()
err := service.addAppLinks(&treeRoot, reqCtx)
require.NoError(t, err)
require.Len(t, treeRoot.Children, 0)
})
t.Run(" Should add all includes when the user is an editor", func(t *testing.T) {
treeRoot := navtree.NavTreeRoot{}
user.Permissions = map[int64]map[string][]string{
1: {pluginaccesscontrol.ActionAppAccess: []string{"*"}},
}
user.OrgRole = identity.RoleEditor
err := service.addAppLinks(&treeRoot, reqCtx)
require.NoError(t, err)
appsNode := treeRoot.FindById(navtree.NavIDApps)
require.Len(t, appsNode.Children, 1)
require.Equal(t, "Test app1 name", appsNode.Children[0].Text)
require.Equal(t, "/a/test-app1/home", appsNode.Children[0].Url)
require.Len(t, appsNode.Children[0].Children, 2)
require.Equal(t, "/a/test-app1/catalog", appsNode.Children[0].Children[0].Url)
require.Equal(t, "/a/test-app1/announcements", appsNode.Children[0].Children[1].Url)
})
t.Run("Should add two includes when the user is a viewer", func(t *testing.T) {
treeRoot := navtree.NavTreeRoot{}
user.Permissions = map[int64]map[string][]string{
1: {pluginaccesscontrol.ActionAppAccess: []string{"*"}},
}
user.OrgRole = identity.RoleViewer
err := service.addAppLinks(&treeRoot, reqCtx)
require.NoError(t, err)
appsNode := treeRoot.FindById(navtree.NavIDApps)
require.Len(t, appsNode.Children, 1)
require.Equal(t, "Test app1 name", appsNode.Children[0].Text)
require.Equal(t, "/a/test-app1/home", appsNode.Children[0].Url)
require.Len(t, appsNode.Children[0].Children, 1)
require.Equal(t, "/a/test-app1/announcements", appsNode.Children[0].Children[0].Url)
})
err := service.addAppLinks(&treeRoot, reqCtx)
require.NoError(t, err)
require.Len(t, treeRoot.Children, 0)
})
t.Run("Should only see the announcements as a none role user with app access", func(t *testing.T) {
treeRoot := navtree.NavTreeRoot{}
user.Permissions = map[int64]map[string][]string{
1: {pluginaccesscontrol.ActionAppAccess: []string{"plugins:id:test-app1"}},
}
user.OrgRole = identity.RoleNone
service.features = featuremgmt.WithFeatures()
t.Run("With plugin RBAC - Enforce action first", func(t *testing.T) {
t.Run("Should not see any includes with no app access", func(t *testing.T) {
treeRoot := navtree.NavTreeRoot{}
user.Permissions = map[int64]map[string][]string{
1: {pluginaccesscontrol.ActionAppAccess: []string{"plugins:id:not-the-test-app1"}},
}
user.OrgRole = identity.RoleNone
service.features = featuremgmt.WithFeatures(featuremgmt.FlagAccessControlOnCall)
err := service.addAppLinks(&treeRoot, reqCtx)
require.NoError(t, err)
appsNode := treeRoot.FindById(navtree.NavIDApps)
require.Len(t, appsNode.Children, 1)
require.Equal(t, "Test app1 name", appsNode.Children[0].Text)
require.Len(t, appsNode.Children[0].Children, 1)
require.Equal(t, "/a/test-app1/announcements", appsNode.Children[0].Children[0].Url)
})
t.Run("Should now see the catalog as a viewer with catalog read", func(t *testing.T) {
treeRoot := navtree.NavTreeRoot{}
user.Permissions = map[int64]map[string][]string{
1: {pluginaccesscontrol.ActionAppAccess: []string{"plugins:id:test-app1"}, catalogReadAction: []string{}},
}
user.OrgRole = identity.RoleViewer
service.features = featuremgmt.WithFeatures()
err := service.addAppLinks(&treeRoot, reqCtx)
require.NoError(t, err)
require.Len(t, treeRoot.Children, 0)
})
t.Run("Should only see the announcements as a none role user with app access", func(t *testing.T) {
treeRoot := navtree.NavTreeRoot{}
user.Permissions = map[int64]map[string][]string{
1: {pluginaccesscontrol.ActionAppAccess: []string{"plugins:id:test-app1"}},
}
user.OrgRole = identity.RoleNone
service.features = featuremgmt.WithFeatures(featuremgmt.FlagAccessControlOnCall)
err := service.addAppLinks(&treeRoot, reqCtx)
require.NoError(t, err)
appsNode := treeRoot.FindById(navtree.NavIDApps)
require.Len(t, appsNode.Children, 1)
require.Equal(t, "Test app1 name", appsNode.Children[0].Text)
require.Equal(t, "/a/test-app1/home", appsNode.Children[0].Url)
require.Len(t, appsNode.Children[0].Children, 2)
require.Equal(t, "/a/test-app1/catalog", appsNode.Children[0].Children[0].Url)
require.Equal(t, "/a/test-app1/announcements", appsNode.Children[0].Children[1].Url)
})
t.Run("Should not see the catalog include as an editor without catalog read", func(t *testing.T) {
treeRoot := navtree.NavTreeRoot{}
user.Permissions = map[int64]map[string][]string{
1: {pluginaccesscontrol.ActionAppAccess: []string{"*"}},
}
user.OrgRole = identity.RoleEditor
service.features = featuremgmt.WithFeatures()
err := service.addAppLinks(&treeRoot, reqCtx)
require.NoError(t, err)
appsNode := treeRoot.FindById(navtree.NavIDApps)
require.Len(t, appsNode.Children, 1)
require.Equal(t, "Test app1 name", appsNode.Children[0].Text)
require.Len(t, appsNode.Children[0].Children, 1)
require.Equal(t, "/a/test-app1/announcements", appsNode.Children[0].Children[0].Url)
})
t.Run("Should now see the catalog as a viewer with catalog read", func(t *testing.T) {
treeRoot := navtree.NavTreeRoot{}
user.Permissions = map[int64]map[string][]string{
1: {pluginaccesscontrol.ActionAppAccess: []string{"plugins:id:test-app1"}, catalogReadAction: []string{}},
}
user.OrgRole = identity.RoleViewer
service.features = featuremgmt.WithFeatures(featuremgmt.FlagAccessControlOnCall)
err := service.addAppLinks(&treeRoot, reqCtx)
require.NoError(t, err)
appsNode := treeRoot.FindById(navtree.NavIDApps)
require.Len(t, appsNode.Children, 1)
require.Equal(t, "Test app1 name", appsNode.Children[0].Text)
require.Equal(t, "/a/test-app1/home", appsNode.Children[0].Url)
require.Len(t, appsNode.Children[0].Children, 2)
require.Equal(t, "/a/test-app1/catalog", appsNode.Children[0].Children[0].Url)
require.Equal(t, "/a/test-app1/announcements", appsNode.Children[0].Children[1].Url)
})
t.Run("Should not see the catalog include as an editor without catalog read", func(t *testing.T) {
treeRoot := navtree.NavTreeRoot{}
user.Permissions = map[int64]map[string][]string{
1: {pluginaccesscontrol.ActionAppAccess: []string{"*"}},
}
user.OrgRole = identity.RoleEditor
service.features = featuremgmt.WithFeatures(featuremgmt.FlagAccessControlOnCall)
err := service.addAppLinks(&treeRoot, reqCtx)
require.NoError(t, err)
appsNode := treeRoot.FindById(navtree.NavIDApps)
require.Len(t, appsNode.Children, 1)
require.Equal(t, "Test app1 name", appsNode.Children[0].Text)
require.Equal(t, "/a/test-app1/home", appsNode.Children[0].Url)
require.Len(t, appsNode.Children[0].Children, 1)
require.Equal(t, "/a/test-app1/announcements", appsNode.Children[0].Children[0].Url)
})
err := service.addAppLinks(&treeRoot, reqCtx)
require.NoError(t, err)
appsNode := treeRoot.FindById(navtree.NavIDApps)
require.Len(t, appsNode.Children, 1)
require.Equal(t, "Test app1 name", appsNode.Children[0].Text)
require.Equal(t, "/a/test-app1/home", appsNode.Children[0].Url)
require.Len(t, appsNode.Children[0].Children, 1)
require.Equal(t, "/a/test-app1/announcements", appsNode.Children[0].Children[0].Url)
})
}