From fc205db46698099e5fce3b59e0f1cde7aeda8a25 Mon Sep 17 00:00:00 2001 From: linoman <2051016+linoman@users.noreply.github.com> Date: Mon, 25 Mar 2024 03:54:45 -0600 Subject: [PATCH] samlsettings: api integration (#84300) * add strategy and tests * use settings provider service and remove multiple providers strategy * Move SAML strategy to ssosettings service * Update codeowners file * reload from settings provider * add saml as configurable provider * Add new SAML strategy * rename old saml settings interface * update saml string references * use OSS license * validate saml provider depends on license for List * add tests for list rendering including saml * change the licensing validation to service init * replace service struct for provider --- pkg/api/login.go | 3 +- pkg/login/social/social.go | 1 + pkg/login/social/socialimpl/service_test.go | 16 +- pkg/services/navtree/navtreeimpl/admin.go | 3 +- .../ssosettings/ssosettingsimpl/service.go | 43 ++-- .../ssosettingsimpl/service_test.go | 184 ++++++++++++++---- .../ssosettings/strategies/saml_strategy.go | 3 +- pkg/setting/provider.go | 4 +- 8 files changed, 193 insertions(+), 64 deletions(-) diff --git a/pkg/api/login.go b/pkg/api/login.go index 15b84597aa3..71030800a12 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/infra/metrics" "github.com/grafana/grafana/pkg/infra/network" + "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/middleware/cookies" "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/auth/identity" @@ -333,7 +334,7 @@ func (hs *HTTPServer) redirectURLWithErrorCookie(c *contextmodel.ReqContext, err } func (hs *HTTPServer) samlEnabled() bool { - return hs.SettingsProvider.KeyValue("auth.saml", "enabled").MustBool(false) && hs.License.FeatureEnabled("saml") + return hs.SettingsProvider.KeyValue("auth.saml", "enabled").MustBool(false) && hs.License.FeatureEnabled(social.SAMLProviderName) } func (hs *HTTPServer) samlName() string { diff --git a/pkg/login/social/social.go b/pkg/login/social/social.go index 0b0ef84c2e6..bf0077cf01e 100644 --- a/pkg/login/social/social.go +++ b/pkg/login/social/social.go @@ -23,6 +23,7 @@ const ( // legacy/old settings for the provider GrafanaNetProviderName = "grafananet" OktaProviderName = "okta" + SAMLProviderName = "saml" ) var ( diff --git a/pkg/login/social/socialimpl/service_test.go b/pkg/login/social/socialimpl/service_test.go index 04fa8b403f0..e8c647c8c0a 100644 --- a/pkg/login/social/socialimpl/service_test.go +++ b/pkg/login/social/socialimpl/service_test.go @@ -14,6 +14,7 @@ import ( "github.com/grafana/grafana/pkg/login/social/connectors" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/licensing" secretsfake "github.com/grafana/grafana/pkg/services/secrets/fakes" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingsimpl" "github.com/grafana/grafana/pkg/services/supportbundles/supportbundlestest" @@ -69,7 +70,18 @@ func TestSocialService_ProvideService(t *testing.T) { accessControl := acimpl.ProvideAccessControl(cfg) sqlStore := db.InitTestDB(t) - ssoSettingsSvc := ssosettingsimpl.ProvideService(cfg, sqlStore, accessControl, routing.NewRouteRegister(), featuremgmt.WithFeatures(), secrets, &usagestats.UsageStatsMock{}, nil) + ssoSettingsSvc := ssosettingsimpl.ProvideService( + cfg, + sqlStore, + accessControl, + routing.NewRouteRegister(), + featuremgmt.WithFeatures(), + secrets, + &usagestats.UsageStatsMock{}, + nil, + &setting.OSSImpl{Cfg: cfg}, + &licensing.OSSLicensingService{}, + ) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -170,7 +182,7 @@ func TestSocialService_ProvideService_GrafanaComGrafanaNet(t *testing.T) { accessControl := acimpl.ProvideAccessControl(cfg) sqlStore := db.InitTestDB(t) - ssoSettingsSvc := ssosettingsimpl.ProvideService(cfg, sqlStore, accessControl, routing.NewRouteRegister(), featuremgmt.WithFeatures(), secrets, &usagestats.UsageStatsMock{}, nil) + ssoSettingsSvc := ssosettingsimpl.ProvideService(cfg, sqlStore, accessControl, routing.NewRouteRegister(), featuremgmt.WithFeatures(), secrets, &usagestats.UsageStatsMock{}, nil, nil, &licensing.OSSLicensingService{}) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { diff --git a/pkg/services/navtree/navtreeimpl/admin.go b/pkg/services/navtree/navtreeimpl/admin.go index 235590b8931..8d3b73dfef6 100644 --- a/pkg/services/navtree/navtreeimpl/admin.go +++ b/pkg/services/navtree/navtreeimpl/admin.go @@ -1,6 +1,7 @@ package navtreeimpl import ( + "github.com/grafana/grafana/pkg/login/social" ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/ssoutils" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" @@ -18,7 +19,7 @@ func (s *ServiceImpl) getAdminNode(c *contextmodel.ReqContext) (*navtree.NavLink hasAccess := ac.HasAccess(s.accessControl, c) hasGlobalAccess := ac.HasGlobalAccess(s.accessControl, s.accesscontrolService, c) orgsAccessEvaluator := ac.EvalPermission(ac.ActionOrgsRead) - authConfigUIAvailable := s.license.FeatureEnabled("saml") || s.cfg.LDAPAuthEnabled + authConfigUIAvailable := s.license.FeatureEnabled(social.SAMLProviderName) || s.cfg.LDAPAuthEnabled // FIXME: If plugin admin is disabled or externally managed, server admins still need to access the page, this is why // while we don't have a permissions for listing plugins the legacy check has to stay as a default diff --git a/pkg/services/ssosettings/ssosettingsimpl/service.go b/pkg/services/ssosettings/ssosettingsimpl/service.go index b753f75cc8d..1d4ae45f55e 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service.go @@ -12,9 +12,11 @@ import ( "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/usagestats" + "github.com/grafana/grafana/pkg/login/social" ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/licensing" "github.com/grafana/grafana/pkg/services/secrets" "github.com/grafana/grafana/pkg/services/ssosettings" "github.com/grafana/grafana/pkg/services/ssosettings/api" @@ -35,29 +37,40 @@ type Service struct { secrets secrets.Service metrics *metrics - fbStrategies []ssosettings.FallbackStrategy - reloadables map[string]ssosettings.Reloadable + fbStrategies []ssosettings.FallbackStrategy + providersList []string + reloadables map[string]ssosettings.Reloadable } func ProvideService(cfg *setting.Cfg, sqlStore db.DB, ac ac.AccessControl, routeRegister routing.RouteRegister, features featuremgmt.FeatureToggles, - secrets secrets.Service, usageStats usagestats.Service, registerer prometheus.Registerer) *Service { - strategies := []ssosettings.FallbackStrategy{ + secrets secrets.Service, usageStats usagestats.Service, registerer prometheus.Registerer, + settingsProvider setting.Provider, licensing licensing.Licensing) *Service { + fbStrategies := []ssosettings.FallbackStrategy{ strategies.NewOAuthStrategy(cfg), - // register other strategies here, for example SAML + } + + providersList := ssosettings.AllOAuthProviders + if licensing.FeatureEnabled(social.SAMLProviderName) { + fbStrategies = append(fbStrategies, strategies.NewSAMLStrategy(settingsProvider)) + + if cfg.SSOSettingsConfigurableProviders[social.SAMLProviderName] { + providersList = append(providersList, social.SAMLProviderName) + } } store := database.ProvideStore(sqlStore) svc := &Service{ - logger: log.New("ssosettings.service"), - cfg: cfg, - store: store, - ac: ac, - fbStrategies: strategies, - secrets: secrets, - metrics: newMetrics(registerer), - reloadables: make(map[string]ssosettings.Reloadable), + logger: log.New("ssosettings.service"), + cfg: cfg, + store: store, + ac: ac, + fbStrategies: fbStrategies, + secrets: secrets, + metrics: newMetrics(registerer), + providersList: providersList, + reloadables: make(map[string]ssosettings.Reloadable), } usageStats.RegisterMetricsFunc(svc.getUsageStats) @@ -114,14 +127,14 @@ func (s *Service) GetForProviderWithRedactedSecrets(ctx context.Context, provide } func (s *Service) List(ctx context.Context) ([]*models.SSOSettings, error) { - result := make([]*models.SSOSettings, 0, len(ssosettings.AllOAuthProviders)) + result := make([]*models.SSOSettings, 0, len(s.providersList)) storedSettings, err := s.store.List(ctx) if err != nil { return nil, err } - for _, provider := range ssosettings.AllOAuthProviders { + for _, provider := range s.providersList { dbSettings := getSettingByProvider(provider, storedSettings) if dbSettings != nil { // Settings are coming from the database thus secrets are encrypted diff --git a/pkg/services/ssosettings/ssosettingsimpl/service_test.go b/pkg/services/ssosettings/ssosettingsimpl/service_test.go index 10fa239fdbb..41066118016 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service_test.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service_test.go @@ -13,19 +13,29 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "gopkg.in/ini.v1" - "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/infra/db/dbtest" + "github.com/grafana/grafana/pkg/infra/usagestats" "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" + "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/licensing/licensingtest" secretsFakes "github.com/grafana/grafana/pkg/services/secrets/fakes" "github.com/grafana/grafana/pkg/services/ssosettings" "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/tests/testsuite" ) +func TestMain(m *testing.M) { + testsuite.Run(m) +} + func TestService_GetForProvider(t *testing.T) { t.Parallel() @@ -231,7 +241,7 @@ func TestService_GetForProvider(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) if tc.setup != nil { tc.setup(env) } @@ -340,7 +350,7 @@ func TestService_GetForProviderWithRedactedSecrets(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) if tc.setup != nil { tc.setup(env) } @@ -368,7 +378,7 @@ func TestService_List(t *testing.T) { wantErr bool }{ { - name: "should return successfully", + name: "should return all oauth providers successfully without saml", setup: func(env testEnv) { env.store.ExpectedSSOSettings = []*models.SSOSettings{ { @@ -491,7 +501,7 @@ func TestService_List(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) if tc.setup != nil { tc.setup(env) } @@ -793,7 +803,7 @@ func TestService_ListWithRedactedSecrets(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) if tc.setup != nil { tc.setup(env) } @@ -817,7 +827,7 @@ func TestService_Upsert(t *testing.T) { t.Run("successfully upsert SSO settings", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := social.AzureADProviderName settings := models.SSOSettings{ @@ -880,7 +890,7 @@ func TestService_Upsert(t *testing.T) { t.Run("returns error if provider is not configurable", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := social.GrafanaComProviderName settings := &models.SSOSettings{ @@ -903,7 +913,7 @@ func TestService_Upsert(t *testing.T) { t.Run("returns error if provider was not found in reloadables", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := social.AzureADProviderName settings := &models.SSOSettings{ @@ -927,7 +937,7 @@ func TestService_Upsert(t *testing.T) { t.Run("returns error if validation fails", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := social.AzureADProviderName settings := models.SSOSettings{ @@ -951,7 +961,7 @@ func TestService_Upsert(t *testing.T) { t.Run("returns error if a fallback strategy is not available for the provider", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) settings := &models.SSOSettings{ Provider: social.AzureADProviderName, @@ -972,7 +982,7 @@ func TestService_Upsert(t *testing.T) { t.Run("returns error if secrets encryption failed", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := social.OktaProviderName settings := models.SSOSettings{ @@ -997,7 +1007,7 @@ func TestService_Upsert(t *testing.T) { t.Run("should not update the current secret if the secret has not been updated", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := social.AzureADProviderName settings := models.SSOSettings{ @@ -1034,7 +1044,7 @@ func TestService_Upsert(t *testing.T) { t.Run("returns error if store failed to upsert settings", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := social.AzureADProviderName settings := models.SSOSettings{ @@ -1066,7 +1076,7 @@ func TestService_Upsert(t *testing.T) { t.Run("successfully upsert SSO settings if reload fails", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := social.AzureADProviderName settings := models.SSOSettings{ @@ -1099,7 +1109,7 @@ func TestService_Delete(t *testing.T) { t.Run("successfully delete SSO settings", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) var wg sync.WaitGroup wg.Add(1) @@ -1137,7 +1147,7 @@ func TestService_Delete(t *testing.T) { t.Run("return error if SSO setting was not found for the specified provider", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := social.AzureADProviderName reloadable := ssosettingstests.NewMockReloadable(t) @@ -1153,7 +1163,7 @@ func TestService_Delete(t *testing.T) { t.Run("should not delete the SSO settings if the provider is not configurable", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) env.cfg.SSOSettingsConfigurableProviders = map[string]bool{social.AzureADProviderName: true} provider := social.GrafanaComProviderName @@ -1166,7 +1176,7 @@ func TestService_Delete(t *testing.T) { t.Run("return error when store fails to delete the SSO settings for the specified provider", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := social.AzureADProviderName env.store.ExpectedError = errors.New("delete sso settings failed") @@ -1179,7 +1189,7 @@ func TestService_Delete(t *testing.T) { t.Run("return successfully when the deletion was successful but reloading the settings fail", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := social.AzureADProviderName reloadable := ssosettingstests.NewMockReloadable(t) @@ -1201,7 +1211,7 @@ func TestService_DoReload(t *testing.T) { t.Run("successfully reload settings", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) settingsList := []*models.SSOSettings{ { @@ -1241,7 +1251,7 @@ func TestService_DoReload(t *testing.T) { t.Run("failed fetching the SSO settings", func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) provider := "github" @@ -1336,7 +1346,7 @@ func TestService_decryptSecrets(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - env := setupTestEnv(t) + env := setupTestEnv(t, false, false, nil) if tc.setup != nil { tc.setup(env) @@ -1357,7 +1367,73 @@ func TestService_decryptSecrets(t *testing.T) { } } -func setupTestEnv(t *testing.T) testEnv { +func Test_ProviderService(t *testing.T) { + tests := []struct { + name string + isLicenseEnabled bool + configurableProviders map[string]bool + expectedProvidersList []string + strategiesLength int + }{ + { + name: "should return all OAuth providers but not saml because the licensing feature is not enabled", + isLicenseEnabled: false, + expectedProvidersList: []string{ + "github", + "gitlab", + "google", + "generic_oauth", + "grafana_com", + "azuread", + "okta", + }, + strategiesLength: 1, + }, + { + name: "should return all fallback strategies and it should return all OAuth providers but not saml because the licensing feature is enabled but the configurable provider is not setup", + isLicenseEnabled: true, + expectedProvidersList: []string{ + "github", + "gitlab", + "google", + "generic_oauth", + "grafana_com", + "azuread", + "okta", + }, + strategiesLength: 2, + }, + { + name: "should return all fallback strategies and it should return all OAuth providers and saml because the licensing feature is enabled and the provider is setup", + isLicenseEnabled: true, + configurableProviders: map[string]bool{"saml": true}, + expectedProvidersList: []string{ + "github", + "gitlab", + "google", + "generic_oauth", + "grafana_com", + "azuread", + "okta", + "saml", + }, + strategiesLength: 2, + }, + } + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + env := setupTestEnv(t, tc.isLicenseEnabled, true, tc.configurableProviders) + + require.Equal(t, tc.expectedProvidersList, env.service.providersList) + require.Equal(t, tc.strategiesLength, len(env.service.fbStrategies)) + }) + } +} + +func setupTestEnv(t *testing.T, isLicensingEnabled, keepFallbackStratergies bool, extraConfigurableProviders map[string]bool) testEnv { t.Helper() store := ssosettingstests.NewFakeStore() @@ -1368,28 +1444,52 @@ func setupTestEnv(t *testing.T) testEnv { fallbackStrategy.ExpectedIsMatch = true - cfg := &setting.Cfg{ - SSOSettingsConfigurableProviders: map[string]bool{ - "github": true, - "okta": true, - "azuread": true, - "google": true, - "generic_oauth": true, - "gitlab": true, - }, + iniFile, _ := ini.Load([]byte("")) + + configurableProviders := map[string]bool{ + "github": true, + "okta": true, + "azuread": true, + "google": true, + "generic_oauth": true, + "gitlab": true, } - svc := &Service{ - logger: log.NewNopLogger(), - cfg: cfg, - store: store, - ac: accessControl, - fbStrategies: []ssosettings.FallbackStrategy{fallbackStrategy}, - reloadables: reloadables, - metrics: newMetrics(prometheus.NewRegistry()), - secrets: secrets, + for k, v := range extraConfigurableProviders { + configurableProviders[k] = v } + cfg := &setting.Cfg{ + SSOSettingsConfigurableProviders: configurableProviders, + Raw: iniFile, + } + + licensing := licensingtest.NewFakeLicensing() + licensing.On("FeatureEnabled", "saml").Return(isLicensingEnabled) + + svc := ProvideService( + cfg, + &dbtest.FakeDB{}, + accessControl, + routing.NewRouteRegister(), + featuremgmt.WithManager(nil), + secretsFakes.NewMockService(t), + &usagestats.UsageStatsMock{}, + prometheus.NewRegistry(), + &setting.OSSImpl{Cfg: cfg}, + licensing, + ) + + // overriding values for exposed fields + svc.store = store + if !keepFallbackStratergies { + svc.fbStrategies = []ssosettings.FallbackStrategy{ + fallbackStrategy, + } + } + svc.secrets = secrets + svc.reloadables = reloadables + return testEnv{ cfg: cfg, service: svc, diff --git a/pkg/services/ssosettings/strategies/saml_strategy.go b/pkg/services/ssosettings/strategies/saml_strategy.go index b8c8dfabf86..3a963674066 100644 --- a/pkg/services/ssosettings/strategies/saml_strategy.go +++ b/pkg/services/ssosettings/strategies/saml_strategy.go @@ -4,6 +4,7 @@ import ( "context" "time" + "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/services/ssosettings" "github.com/grafana/grafana/pkg/setting" ) @@ -21,7 +22,7 @@ func NewSAMLStrategy(settingsProvider setting.Provider) *SAMLStrategy { } func (s *SAMLStrategy) IsMatch(provider string) bool { - return provider == "saml" + return provider == social.SAMLProviderName } func (s *SAMLStrategy) GetProviderConfig(_ context.Context, provider string) (map[string]any, error) { diff --git a/pkg/setting/provider.go b/pkg/setting/provider.go index a66bef430c2..1978689f40b 100644 --- a/pkg/setting/provider.go +++ b/pkg/setting/provider.go @@ -85,11 +85,11 @@ type KeyValue interface { // service that have support for configuration reloads. type ReloadHandler interface { // Reload handles reloading of configuration changes. - Reload(section Section) error + ReloadSection(section Section) error // Validate validates the configuration, if the validation // fails the configuration will not be updated neither reloaded. - Validate(section Section) error + ValidateSection(section Section) error } type SettingsBag map[string]map[string]string