From 58efa49933f609449ddd32502dbc052e4bbd5050 Mon Sep 17 00:00:00 2001 From: Ieva Date: Tue, 5 Sep 2023 11:04:39 +0100 Subject: [PATCH] Chore: remove `IsDisabled` method for access control (#74340) remove IsDisabled method for access control, clean up tests --- pkg/api/frontendsettings.go | 2 +- pkg/api/frontendsettings_test.go | 2 +- pkg/api/plugins_test.go | 1 - pkg/services/accesscontrol/accesscontrol.go | 9 -------- .../accesscontrol/acimpl/accesscontrol.go | 4 ---- pkg/services/accesscontrol/acimpl/service.go | 4 ---- pkg/services/accesscontrol/actest/fake.go | 10 --------- pkg/services/accesscontrol/mock/mock.go | 22 ------------------- .../ngalert/api/api_prometheus_test.go | 2 +- pkg/services/ngalert/api/api_testing_test.go | 2 +- pkg/services/ngalert/api/util_test.go | 2 +- pkg/services/quota/quotaimpl/quota_test.go | 2 +- .../kvstore/migrations/datasource_mig_test.go | 2 +- pkg/setting/setting.go | 11 ++++------ 14 files changed, 11 insertions(+), 64 deletions(-) diff --git a/pkg/api/frontendsettings.go b/pkg/api/frontendsettings.go index cd842ca61a5..4341c614ffc 100644 --- a/pkg/api/frontendsettings.go +++ b/pkg/api/frontendsettings.go @@ -120,7 +120,7 @@ func (hs *HTTPServer) getFrontendSettings(c *contextmodel.ReqContext) (*dtos.Fro VerifyEmailEnabled: setting.VerifyEmailEnabled, SigV4AuthEnabled: setting.SigV4AuthEnabled, AzureAuthEnabled: setting.AzureAuthEnabled, - RbacEnabled: hs.Cfg.RBACEnabled, + RbacEnabled: true, ExploreEnabled: setting.ExploreEnabled, HelpEnabled: setting.HelpEnabled, ProfileEnabled: setting.ProfileEnabled, diff --git a/pkg/api/frontendsettings_test.go b/pkg/api/frontendsettings_test.go index 6f0136bfc59..79206bd99e8 100644 --- a/pkg/api/frontendsettings_test.go +++ b/pkg/api/frontendsettings_test.go @@ -68,7 +68,7 @@ func setupTestEnvironment(t *testing.T, cfg *setting.Cfg, features *featuremgmt. SettingsProvider: setting.ProvideProvider(cfg), pluginStore: pluginStore, grafanaUpdateChecker: &updatechecker.GrafanaService{}, - AccessControl: accesscontrolmock.New().WithDisabled(), + AccessControl: accesscontrolmock.New(), PluginSettings: pluginsSettings, pluginsCDNService: pluginscdn.ProvideService(&config.Cfg{ PluginsCDNURLTemplate: cfg.PluginsCDNURLTemplate, diff --git a/pkg/api/plugins_test.go b/pkg/api/plugins_test.go index 6e1bb8c627c..b21701a1c90 100644 --- a/pkg/api/plugins_test.go +++ b/pkg/api/plugins_test.go @@ -65,7 +65,6 @@ func Test_PluginsInstallAndUninstall(t *testing.T) { for _, tc := range tcs { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = &setting.Cfg{ - RBACEnabled: true, PluginAdminEnabled: tc.pluginAdminEnabled, PluginAdminExternalManageEnabled: tc.pluginAdminExternalManageEnabled} hs.orgService = &orgtest.FakeOrgService{ExpectedOrg: &org.Org{}} diff --git a/pkg/services/accesscontrol/accesscontrol.go b/pkg/services/accesscontrol/accesscontrol.go index c0b1efab32c..24cde5f83f1 100644 --- a/pkg/services/accesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/accesscontrol.go @@ -10,7 +10,6 @@ import ( contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" - "github.com/grafana/grafana/pkg/setting" ) type AccessControl interface { @@ -19,8 +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) - //IsDisabled returns if access control is enabled or not - IsDisabled() bool } type Service interface { @@ -43,8 +40,6 @@ type Service interface { SaveExternalServiceRole(ctx context.Context, cmd SaveExternalServiceRoleCommand) error // DeleteExternalServiceRole removes an external service's role and its assignment. DeleteExternalServiceRole(ctx context.Context, externalServiceID string) error - //IsDisabled returns if access control is enabled or not - IsDisabled() bool } type RoleRegistry interface { @@ -371,10 +366,6 @@ func ManagedBuiltInRoleName(builtInRole string) string { return fmt.Sprintf("managed:builtins:%s:permissions", strings.ToLower(builtInRole)) } -func IsDisabled(cfg *setting.Cfg) bool { - return !cfg.RBACEnabled -} - // GetOrgRoles returns legacy org roles for a user func GetOrgRoles(user identity.Requester) []string { roles := []string{string(user.GetOrgRole())} diff --git a/pkg/services/accesscontrol/acimpl/accesscontrol.go b/pkg/services/accesscontrol/acimpl/accesscontrol.go index e7168f3770e..cd4919e984b 100644 --- a/pkg/services/accesscontrol/acimpl/accesscontrol.go +++ b/pkg/services/accesscontrol/acimpl/accesscontrol.go @@ -63,7 +63,3 @@ func (a *AccessControl) Evaluate(ctx context.Context, user identity.Requester, e func (a *AccessControl) RegisterScopeAttributeResolver(prefix string, resolver accesscontrol.ScopeAttributeResolver) { a.resolvers.AddScopeAttributeResolver(prefix, resolver) } - -func (a *AccessControl) IsDisabled() bool { - return accesscontrol.IsDisabled(a.cfg) -} diff --git a/pkg/services/accesscontrol/acimpl/service.go b/pkg/services/accesscontrol/acimpl/service.go index 4ad93f2da63..6f2f9b7f072 100644 --- a/pkg/services/accesscontrol/acimpl/service.go +++ b/pkg/services/accesscontrol/acimpl/service.go @@ -215,10 +215,6 @@ func (s *Service) RegisterFixedRoles(ctx context.Context) error { return nil } -func (s *Service) IsDisabled() bool { - return accesscontrol.IsDisabled(s.cfg) -} - func permissionCacheKey(user identity.Requester) (string, error) { key, err := user.GetCacheKey() if err != nil { diff --git a/pkg/services/accesscontrol/actest/fake.go b/pkg/services/accesscontrol/actest/fake.go index d3613417b18..87113fb6e3f 100644 --- a/pkg/services/accesscontrol/actest/fake.go +++ b/pkg/services/accesscontrol/actest/fake.go @@ -12,7 +12,6 @@ var _ accesscontrol.RoleRegistry = new(FakeService) type FakeService struct { ExpectedErr error - ExpectedDisabled bool ExpectedCachedPermissions bool ExpectedPermissions []accesscontrol.Permission ExpectedFilteredUserPermissions []accesscontrol.Permission @@ -49,10 +48,6 @@ func (f FakeService) RegisterFixedRoles(ctx context.Context) error { return f.ExpectedErr } -func (f FakeService) IsDisabled() bool { - return f.ExpectedDisabled -} - func (f FakeService) SaveExternalServiceRole(ctx context.Context, cmd accesscontrol.SaveExternalServiceRoleCommand) error { return f.ExpectedErr } @@ -65,7 +60,6 @@ var _ accesscontrol.AccessControl = new(FakeAccessControl) type FakeAccessControl struct { ExpectedErr error - ExpectedDisabled bool ExpectedEvaluate bool } @@ -76,10 +70,6 @@ func (f FakeAccessControl) Evaluate(ctx context.Context, user identity.Requester func (f FakeAccessControl) RegisterScopeAttributeResolver(prefix string, resolver accesscontrol.ScopeAttributeResolver) { } -func (f FakeAccessControl) IsDisabled() bool { - return f.ExpectedDisabled -} - type FakeStore struct { ExpectedUserPermissions []accesscontrol.Permission ExpectedUsersPermissions map[int64][]accesscontrol.Permission diff --git a/pkg/services/accesscontrol/mock/mock.go b/pkg/services/accesscontrol/mock/mock.go index 6e1b4ad5fb0..8043cc5280a 100644 --- a/pkg/services/accesscontrol/mock/mock.go +++ b/pkg/services/accesscontrol/mock/mock.go @@ -22,7 +22,6 @@ type Calls struct { Evaluate []interface{} GetUserPermissions []interface{} ClearUserPermissionCache []interface{} - IsDisabled []interface{} DeclareFixedRoles []interface{} DeclarePluginRoles []interface{} GetUserBuiltInRoles []interface{} @@ -38,8 +37,6 @@ type Calls struct { type Mock struct { // Unless an override is provided, permissions will be returned by GetUserPermissions permissions []accesscontrol.Permission - // Unless an override is provided, disabled will be returned by IsDisabled - disabled bool // Unless an override is provided, builtInRoles will be returned by GetUserBuiltInRoles builtInRoles []string @@ -50,7 +47,6 @@ type Mock struct { EvaluateFunc func(context.Context, *user.SignedInUser, accesscontrol.Evaluator) (bool, error) GetUserPermissionsFunc func(context.Context, *user.SignedInUser, accesscontrol.Options) ([]accesscontrol.Permission, error) ClearUserPermissionCacheFunc func(*user.SignedInUser) - IsDisabledFunc func() bool DeclareFixedRolesFunc func(...accesscontrol.RoleRegistration) error DeclarePluginRolesFunc func(context.Context, string, string, []plugins.RoleRegistration) error GetUserBuiltInRolesFunc func(user *user.SignedInUser) []string @@ -72,7 +68,6 @@ var _ fullAccessControl = New() func New() *Mock { mock := &Mock{ Calls: Calls{}, - disabled: false, permissions: []accesscontrol.Permission{}, builtInRoles: []string{}, scopeResolvers: accesscontrol.NewResolvers(log.NewNopLogger()), @@ -90,11 +85,6 @@ func (m *Mock) WithPermissions(permissions []accesscontrol.Permission) *Mock { return m } -func (m *Mock) WithDisabled() *Mock { - m.disabled = true - return m -} - func (m *Mock) WithBuiltInRoles(builtInRoles []string) *Mock { m.builtInRoles = builtInRoles return m @@ -160,18 +150,6 @@ func (m *Mock) ClearUserPermissionCache(usr identity.Requester) { } } -// Middleware checks if service disabled or not to switch to fallback authorization. -// This mock return m.disabled unless an override is provided. -func (m *Mock) IsDisabled() bool { - m.Calls.IsDisabled = append(m.Calls.IsDisabled, struct{}{}) - // Use override if provided - if m.IsDisabledFunc != nil { - return m.IsDisabledFunc() - } - // Otherwise return the Disabled bool - return m.disabled -} - // DeclareFixedRoles allow the caller to declare, to the service, fixed roles and their // assignments to organization roles ("Viewer", "Editor", "Admin") or "Grafana Admin" // This mock returns no error unless an override is provided. diff --git a/pkg/services/ngalert/api/api_prometheus_test.go b/pkg/services/ngalert/api/api_prometheus_test.go index 672da5e78e5..5a34b2f0b69 100644 --- a/pkg/services/ngalert/api/api_prometheus_test.go +++ b/pkg/services/ngalert/api/api_prometheus_test.go @@ -502,7 +502,7 @@ func TestRouteGetRuleStatuses(t *testing.T) { log: log.NewNopLogger(), manager: fakeAIM, store: ruleStore, - ac: acmock.New().WithDisabled(), + ac: acmock.New(), } response := api.RouteGetRuleStatuses(c) diff --git a/pkg/services/ngalert/api/api_testing_test.go b/pkg/services/ngalert/api/api_testing_test.go index 26a4299ff18..f3e6924a2dc 100644 --- a/pkg/services/ngalert/api/api_testing_test.go +++ b/pkg/services/ngalert/api/api_testing_test.go @@ -271,7 +271,7 @@ func TestRouteEvalQueries(t *testing.T) { func createTestingApiSrv(t *testing.T, ds *fakes.FakeCacheService, ac *acMock.Mock, evaluator eval.EvaluatorFactory) *TestingApiSrv { if ac == nil { - ac = acMock.New().WithDisabled() + ac = acMock.New() } return &TestingApiSrv{ diff --git a/pkg/services/ngalert/api/util_test.go b/pkg/services/ngalert/api/util_test.go index 92fad5cce6e..55f5731aa5c 100644 --- a/pkg/services/ngalert/api/util_test.go +++ b/pkg/services/ngalert/api/util_test.go @@ -60,7 +60,7 @@ func TestAlertingProxy_createProxyContext(t *testing.T) { t.Run("should create a copy of request context", func(t *testing.T) { for _, mock := range []*accesscontrolmock.Mock{ - accesscontrolmock.New(), accesscontrolmock.New().WithDisabled(), + accesscontrolmock.New(), accesscontrolmock.New(), } { proxy := AlertingProxy{ DataProxy: nil, diff --git a/pkg/services/quota/quotaimpl/quota_test.go b/pkg/services/quota/quotaimpl/quota_test.go index 7883767ef65..505ce6b79ba 100644 --- a/pkg/services/quota/quotaimpl/quota_test.go +++ b/pkg/services/quota/quotaimpl/quota_test.go @@ -475,7 +475,7 @@ func setupEnv(t *testing.T, sqlStore *sqlstore.SQLStore, b bus.Bus, quotaService require.NoError(t, err) secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore()) secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger")) - _, err = dsservice.ProvideService(sqlStore, secretsService, secretsStore, sqlStore.Cfg, featuremgmt.WithFeatures(), acmock.New().WithDisabled(), acmock.NewMockedPermissionsService(), quotaService) + _, err = dsservice.ProvideService(sqlStore, secretsService, secretsStore, sqlStore.Cfg, featuremgmt.WithFeatures(), acmock.New(), acmock.NewMockedPermissionsService(), quotaService) require.NoError(t, err) m := metrics.NewNGAlert(prometheus.NewRegistry()) ruleStore, err := ngstore.ProvideDBStore(sqlStore.Cfg, featuremgmt.WithFeatures(), sqlStore, &foldertest.FakeService{}, diff --git a/pkg/services/secrets/kvstore/migrations/datasource_mig_test.go b/pkg/services/secrets/kvstore/migrations/datasource_mig_test.go index 9de63533748..bde1a10c72d 100644 --- a/pkg/services/secrets/kvstore/migrations/datasource_mig_test.go +++ b/pkg/services/secrets/kvstore/migrations/datasource_mig_test.go @@ -30,7 +30,7 @@ func SetupTestDataSourceSecretMigrationService(t *testing.T, sqlStore db.DB, kvS } secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore()) quotaService := quotatest.New(false, nil) - dsService, err := dsservice.ProvideService(sqlStore, secretsService, secretsStore, cfg, features, acmock.New().WithDisabled(), acmock.NewMockedPermissionsService(), quotaService) + dsService, err := dsservice.ProvideService(sqlStore, secretsService, secretsStore, cfg, features, acmock.New(), acmock.NewMockedPermissionsService(), quotaService) require.NoError(t, err) migService := ProvideDataSourceMigrationService(dsService, kvStore, features) return migService diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index 561cd916b5c..57cbfff3f07 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -536,7 +536,6 @@ type Cfg struct { OAuth2ServerAccessTokenLifespan time.Duration // Access Control - RBACEnabled bool RBACPermissionCache bool // Enable Permission validation during role creation and provisioning RBACPermissionValidationEnabled bool @@ -970,11 +969,10 @@ var skipStaticRootValidation = false func NewCfg() *Cfg { return &Cfg{ - Target: []string{"all"}, - Logger: log.New("settings"), - Raw: ini.Empty(), - Azure: &azsettings.AzureSettings{}, - RBACEnabled: true, + Target: []string{"all"}, + Logger: log.New("settings"), + Raw: ini.Empty(), + Azure: &azsettings.AzureSettings{}, } } @@ -1659,7 +1657,6 @@ func readAuthSettings(iniFile *ini.File, cfg *Cfg) (err error) { func readAccessControlSettings(iniFile *ini.File, cfg *Cfg) { rbac := iniFile.Section("rbac") - cfg.RBACEnabled = true cfg.RBACPermissionCache = rbac.Key("permission_cache").MustBool(true) cfg.RBACPermissionValidationEnabled = rbac.Key("permission_validation_enabled").MustBool(false) cfg.RBACResetBasicRoles = rbac.Key("reset_basic_roles").MustBool(false)