From 5a1b9d2283385b9b44c0fd4f14cd7865931975a2 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Fri, 26 Aug 2022 09:59:34 +0200 Subject: [PATCH] RBAC: Remove DeclareFixedRoles wrapper on Access control and inject service (#54153) * RBAC: Remove DeclareFixedRoles wrapper on Access control and inject service when needed --- pkg/api/accesscontrol.go | 4 +- pkg/plugins/accesscontrol.go | 4 +- pkg/services/accesscontrol/accesscontrol.go | 4 -- pkg/services/accesscontrol/actest/fake.go | 4 -- .../ossaccesscontrol/accesscontrol.go | 5 --- pkg/services/ngalert/accesscontrol.go | 4 +- pkg/services/ngalert/ngalert.go | 42 ++++++++++--------- pkg/services/ngalert/tests/util.go | 2 +- pkg/services/serviceaccounts/manager/roles.go | 4 +- .../serviceaccounts/manager/service.go | 3 +- 10 files changed, 33 insertions(+), 43 deletions(-) diff --git a/pkg/api/accesscontrol.go b/pkg/api/accesscontrol.go index d1cbbc74b6d..5c6932c7d49 100644 --- a/pkg/api/accesscontrol.go +++ b/pkg/api/accesscontrol.go @@ -43,7 +43,7 @@ var ( // that HTTPServer needs func (hs *HTTPServer) declareFixedRoles() error { // Declare plugins roles - if err := plugins.DeclareRBACRoles(hs.AccessControl); err != nil { + if err := plugins.DeclareRBACRoles(hs.accesscontrolService); err != nil { return err } @@ -419,7 +419,7 @@ func (hs *HTTPServer) declareFixedRoles() error { Grants: []string{"Admin"}, } - return hs.AccessControl.DeclareFixedRoles( + return hs.accesscontrolService.DeclareFixedRoles( provisioningWriterRole, datasourcesReaderRole, builtInDatasourceReader, datasourcesWriterRole, datasourcesIdReaderRole, orgReaderRole, orgWriterRole, orgMaintainerRole, teamsCreatorRole, teamsWriterRole, datasourcesExplorerRole, diff --git a/pkg/plugins/accesscontrol.go b/pkg/plugins/accesscontrol.go index c382200d79e..73aaaae5e25 100644 --- a/pkg/plugins/accesscontrol.go +++ b/pkg/plugins/accesscontrol.go @@ -13,7 +13,7 @@ var ( ScopeProvider = ac.NewScopeProvider("plugins") ) -func DeclareRBACRoles(acService ac.AccessControl) error { +func DeclareRBACRoles(service ac.Service) error { AppPluginsReader := ac.RoleRegistration{ Role: ac.RoleDTO{ Name: ac.FixedRolePrefix + "plugins.app:reader", @@ -26,5 +26,5 @@ func DeclareRBACRoles(acService ac.AccessControl) error { }, Grants: []string{string(org.RoleViewer)}, } - return acService.DeclareFixedRoles(AppPluginsReader) + return service.DeclareFixedRoles(AppPluginsReader) } diff --git a/pkg/services/accesscontrol/accesscontrol.go b/pkg/services/accesscontrol/accesscontrol.go index 0d2d6b13efc..eb5ffb8e10f 100644 --- a/pkg/services/accesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/accesscontrol.go @@ -18,10 +18,6 @@ type AccessControl interface { // RegisterScopeAttributeResolver allows the caller to register a scope resolver for a // specific scope prefix (ex: datasources:name:) RegisterScopeAttributeResolver(prefix string, resolver ScopeAttributeResolver) - // DeclareFixedRoles allows the caller to declare, to the service, fixed roles and their - // assignments to organization roles ("Viewer", "Editor", "Admin") or "Grafana Admin" - // FIXME: Remove from access control interface and inject service where this is needed - DeclareFixedRoles(registrations ...RoleRegistration) error //IsDisabled returns if access control is enabled or not IsDisabled() bool } diff --git a/pkg/services/accesscontrol/actest/fake.go b/pkg/services/accesscontrol/actest/fake.go index 58cf5d8bb0c..66fffd38079 100644 --- a/pkg/services/accesscontrol/actest/fake.go +++ b/pkg/services/accesscontrol/actest/fake.go @@ -55,10 +55,6 @@ func (f FakeAccessControl) Evaluate(ctx context.Context, user *user.SignedInUser func (f FakeAccessControl) RegisterScopeAttributeResolver(prefix string, resolver accesscontrol.ScopeAttributeResolver) { } -func (f FakeAccessControl) DeclareFixedRoles(registrations ...accesscontrol.RoleRegistration) error { - return f.ExpectedErr -} - func (f FakeAccessControl) IsDisabled() bool { return f.ExpectedDisabled } diff --git a/pkg/services/accesscontrol/ossaccesscontrol/accesscontrol.go b/pkg/services/accesscontrol/ossaccesscontrol/accesscontrol.go index 0c3279962e5..79bc7575592 100644 --- a/pkg/services/accesscontrol/ossaccesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/ossaccesscontrol/accesscontrol.go @@ -66,11 +66,6 @@ func (a *AccessControl) RegisterScopeAttributeResolver(prefix string, resolver a a.resolvers.AddScopeAttributeResolver(prefix, resolver) } -func (a *AccessControl) DeclareFixedRoles(registrations ...accesscontrol.RoleRegistration) error { - // FIXME: Remove wrapped call - return a.service.DeclareFixedRoles(registrations...) -} - func (a *AccessControl) IsDisabled() bool { return accesscontrol.IsDisabled(a.cfg) } diff --git a/pkg/services/ngalert/accesscontrol.go b/pkg/services/ngalert/accesscontrol.go index ca53b60ba29..f583a9ae47a 100644 --- a/pkg/services/ngalert/accesscontrol.go +++ b/pkg/services/ngalert/accesscontrol.go @@ -173,8 +173,8 @@ var ( } ) -func DeclareFixedRoles(ac accesscontrol.AccessControl) error { - return ac.DeclareFixedRoles( +func DeclareFixedRoles(service accesscontrol.Service) error { + return service.DeclareFixedRoles( rulesReaderRole, rulesWriterRole, instancesReaderRole, instancesWriterRole, notificationsReaderRole, notificationsWriterRole, diff --git a/pkg/services/ngalert/ngalert.go b/pkg/services/ngalert/ngalert.go index ca0de40a6cc..4adf7d008c6 100644 --- a/pkg/services/ngalert/ngalert.go +++ b/pkg/services/ngalert/ngalert.go @@ -41,26 +41,27 @@ func ProvideService(cfg *setting.Cfg, dataSourceCache datasources.CacheService, sqlStore *sqlstore.SQLStore, kvStore kvstore.KVStore, expressionService *expr.Service, dataProxy *datasourceproxy.DataSourceProxyService, quotaService quota.Service, secretsService secrets.Service, notificationService notifications.Service, m *metrics.NGAlert, folderService dashboards.FolderService, ac accesscontrol.AccessControl, dashboardService dashboards.DashboardService, renderService rendering.Service, - bus bus.Bus) (*AlertNG, error) { + bus bus.Bus, accesscontrolService accesscontrol.Service) (*AlertNG, error) { ng := &AlertNG{ - Cfg: cfg, - DataSourceCache: dataSourceCache, - DataSourceService: dataSourceService, - RouteRegister: routeRegister, - SQLStore: sqlStore, - KVStore: kvStore, - ExpressionService: expressionService, - DataProxy: dataProxy, - QuotaService: quotaService, - SecretsService: secretsService, - Metrics: m, - Log: log.New("ngalert"), - NotificationService: notificationService, - folderService: folderService, - accesscontrol: ac, - dashboardService: dashboardService, - renderService: renderService, - bus: bus, + Cfg: cfg, + DataSourceCache: dataSourceCache, + DataSourceService: dataSourceService, + RouteRegister: routeRegister, + SQLStore: sqlStore, + KVStore: kvStore, + ExpressionService: expressionService, + DataProxy: dataProxy, + QuotaService: quotaService, + SecretsService: secretsService, + Metrics: m, + Log: log.New("ngalert"), + NotificationService: notificationService, + folderService: folderService, + accesscontrol: ac, + dashboardService: dashboardService, + renderService: renderService, + bus: bus, + accesscontrolService: accesscontrolService, } if ng.IsDisabled() { @@ -100,6 +101,7 @@ type AlertNG struct { MultiOrgAlertmanager *notifier.MultiOrgAlertmanager AlertsRouter *sender.AlertsRouter accesscontrol accesscontrol.AccessControl + accesscontrolService accesscontrol.Service bus bus.Bus } @@ -211,7 +213,7 @@ func (ng *AlertNG) init() error { } api.RegisterAPIEndpoints(ng.Metrics.GetAPIMetrics()) - return DeclareFixedRoles(ng.accesscontrol) + return DeclareFixedRoles(ng.accesscontrolService) } func subscribeToFolderChanges(logger log.Logger, bus bus.Bus, dbStore store.RuleStore, scheduler schedule.ScheduleService) { diff --git a/pkg/services/ngalert/tests/util.go b/pkg/services/ngalert/tests/util.go index bcd92a8a281..ea3415c7ec7 100644 --- a/pkg/services/ngalert/tests/util.go +++ b/pkg/services/ngalert/tests/util.go @@ -64,7 +64,7 @@ func SetupTestEnv(t *testing.T, baseInterval time.Duration) (*ngalert.AlertNG, * ng, err := ngalert.ProvideService( cfg, nil, nil, routing.NewRouteRegister(), sqlStore, nil, nil, nil, nil, - secretsService, nil, m, folderService, ac, &dashboards.FakeDashboardService{}, nil, bus, + secretsService, nil, m, folderService, ac, &dashboards.FakeDashboardService{}, nil, bus, ac, ) require.NoError(t, err) return ng, &store.DBstore{ diff --git a/pkg/services/serviceaccounts/manager/roles.go b/pkg/services/serviceaccounts/manager/roles.go index 6b958716cba..e78112c0544 100644 --- a/pkg/services/serviceaccounts/manager/roles.go +++ b/pkg/services/serviceaccounts/manager/roles.go @@ -6,7 +6,7 @@ import ( "github.com/grafana/grafana/pkg/services/serviceaccounts" ) -func RegisterRoles(ac accesscontrol.AccessControl) error { +func RegisterRoles(service accesscontrol.Service) error { saReader := accesscontrol.RoleRegistration{ Role: accesscontrol.RoleDTO{ Name: "fixed:serviceaccounts:reader", @@ -69,7 +69,7 @@ func RegisterRoles(ac accesscontrol.AccessControl) error { Grants: []string{string(org.RoleAdmin)}, } - if err := ac.DeclareFixedRoles(saReader, saCreator, saWriter); err != nil { + if err := service.DeclareFixedRoles(saReader, saCreator, saWriter); err != nil { return err } diff --git a/pkg/services/serviceaccounts/manager/service.go b/pkg/services/serviceaccounts/manager/service.go index 9e1f362d203..131c0998d12 100644 --- a/pkg/services/serviceaccounts/manager/service.go +++ b/pkg/services/serviceaccounts/manager/service.go @@ -31,6 +31,7 @@ func ProvideServiceAccountsService( usageStats usagestats.Service, serviceAccountsStore serviceaccounts.Store, permissionService accesscontrol.ServiceAccountPermissionsService, + accesscontrolService accesscontrol.Service, ) (*ServiceAccountsService, error) { s := &ServiceAccountsService{ store: serviceAccountsStore, @@ -38,7 +39,7 @@ func ProvideServiceAccountsService( backgroundLog: log.New("serviceaccounts.background"), } - if err := RegisterRoles(ac); err != nil { + if err := RegisterRoles(accesscontrolService); err != nil { s.log.Error("Failed to register roles", "error", err) }