Auth: Include missing SSO settings from system settings on read paths (#80421)

* first touches

* Merge missing SSO settings to support Advanced Auth pages

* fix

pkg/services/ssosettings/ssosettingsimpl/service.go

@@ -24,7 +24,7 @@ import (
 var _ ssosettings.Service = (*SSOSettingsService)(nil)
 
 type SSOSettingsService struct {
-	log     log.Logger
+	logger  log.Logger
 	cfg     *setting.Cfg
 	store   ssosettings.Store
 	ac      ac.AccessControl
@@ -45,7 +45,7 @@ func ProvideService(cfg *setting.Cfg, sqlStore db.DB, ac ac.AccessControl,
 	store := database.ProvideStore(sqlStore)
 
 	svc := &SSOSettingsService{
-		log:          log.New("ssosettings.service"),
+		logger:       log.New("ssosettings.service"),
 		cfg:          cfg,
 		store:        store,
 		ac:           ac,
@@ -65,24 +65,17 @@ func ProvideService(cfg *setting.Cfg, sqlStore db.DB, ac ac.AccessControl,
 var _ ssosettings.Service = (*SSOSettingsService)(nil)
 
 func (s *SSOSettingsService) GetForProvider(ctx context.Context, provider string) (*models.SSOSettings, error) {
-	storeSettings, err := s.store.Get(ctx, provider)
-
-	if errors.Is(err, ssosettings.ErrNotFound) {
-		settings, err := s.loadSettingsUsingFallbackStrategy(ctx, provider)
-		if err != nil {
-			return nil, err
-		}
-
-		return settings, nil
+	dbSettings, err := s.store.Get(ctx, provider)
+	if err != nil && !errors.Is(err, ssosettings.ErrNotFound) {
+		return nil, err
 	}
 
+	systemSettings, err := s.loadSettingsUsingFallbackStrategy(ctx, provider)
 	if err != nil {
 		return nil, err
 	}
 
-	storeSettings.Source = models.DB
-
-	return storeSettings, nil
+	return s.mergeSSOSettings(dbSettings, systemSettings), nil
 }
 
 func (s *SSOSettingsService) GetForProviderWithRedactedSecrets(ctx context.Context, provider string) (*models.SSOSettings, error) {
@@ -93,7 +86,7 @@ func (s *SSOSettingsService) GetForProviderWithRedactedSecrets(ctx context.Conte
 
 	for k, v := range storeSettings.Settings {
 		if isSecret(k) && v != "" {
-			storeSettings.Settings[k] = "*********"
+			storeSettings.Settings[k] = setting.RedactedPassword
 		}
 	}
 
@@ -109,17 +102,13 @@ func (s *SSOSettingsService) List(ctx context.Context) ([]*models.SSOSettings, e
 	}
 
 	for _, provider := range ssosettings.AllOAuthProviders {
-		settings := getSettingsByProvider(provider, storedSettings)
-		if len(settings) == 0 {
-			// If there is no data in the DB then we need to load the settings using the fallback strategy
-			fallbackSettings, err := s.loadSettingsUsingFallbackStrategy(ctx, provider)
-			if err != nil {
-				return nil, err
-			}
-
-			settings = append(settings, fallbackSettings)
+		dbSettings := getSettingByProvider(provider, storedSettings)
+		fallbackSettings, err := s.loadSettingsUsingFallbackStrategy(ctx, provider)
+		if err != nil {
+			return nil, err
 		}
-		result = append(result, settings...)
+
+		result = append(result, s.mergeSSOSettings(dbSettings, fallbackSettings))
 	}
 
 	return result, nil
@@ -140,15 +129,6 @@ func (s *SSOSettingsService) Upsert(ctx context.Context, settings models.SSOSett
 		return err
 	}
 
-	systemSettings, err := s.loadSettingsUsingFallbackStrategy(ctx, settings.Provider)
-	if err != nil {
-		return err
-	}
-
-	// add the SSO settings from system that are not available in the user input
-	// in order to have a complete set of SSO settings for every provider in the database
-	settings.Settings = mergeSettings(settings.Settings, systemSettings.Settings)
-
 	settings.Settings, err = s.encryptSecrets(ctx, settings.Settings)
 	if err != nil {
 		return err
@@ -162,7 +142,7 @@ func (s *SSOSettingsService) Upsert(ctx context.Context, settings models.SSOSett
 	go func() {
 		err = social.Reload(context.Background(), settings)
 		if err != nil {
-			s.log.Error("failed to reload the provider", "provider", settings.Provider, "error", err)
+			s.logger.Error("failed to reload the provider", "provider", settings.Provider, "error", err)
 		}
 	}()
 
@@ -210,14 +190,13 @@ func (s *SSOSettingsService) loadSettingsUsingFallbackStrategy(ctx context.Conte
 	}, nil
 }
 
-func getSettingsByProvider(provider string, settings []*models.SSOSettings) []*models.SSOSettings {
-	result := make([]*models.SSOSettings, 0)
+func getSettingByProvider(provider string, settings []*models.SSOSettings) *models.SSOSettings {
 	for _, item := range settings {
 		if item.Provider == provider {
-			result = append(result, item)
+			return item
 		}
 	}
-	return result
+	return nil
 }
 
 func (s *SSOSettingsService) getFallbackStrategyFor(provider string) (ssosettings.FallbackStrategy, bool) {
@@ -273,27 +252,59 @@ func (s *SSOSettingsService) Run(ctx context.Context) error {
 }
 
 func (s *SSOSettingsService) doReload(ctx context.Context) {
-	s.log.Debug("reloading SSO Settings for all providers")
+	s.logger.Debug("reloading SSO Settings for all providers")
 
 	settingsList, err := s.List(ctx)
 	if err != nil {
-		s.log.Error("failed to fetch SSO Settings for all providers", "err", err)
+		s.logger.Error("failed to fetch SSO Settings for all providers", "err", err)
 		return
 	}
 
 	for provider, connector := range s.reloadables {
-		settings := getSettingsByProvider(provider, settingsList)
+		setting := getSettingByProvider(provider, settingsList)
 
-		if len(settings) > 0 {
-			err = connector.Reload(ctx, *settings[0])
-			if err != nil {
-				s.log.Error("failed to reload SSO Settings", "provider", provider, "err", err)
-				continue
-			}
+		err = connector.Reload(ctx, *setting)
+		if err != nil {
+			s.logger.Error("failed to reload SSO Settings", "provider", provider, "err", err)
+			continue
 		}
 	}
 }
 
+// mergeSSOSettings merges the settings from the database with the system settings
+// Required because it is possible that the user has configured some of the settings (current Advanced OAuth settings)
+// and the rest of the settings are loaded from the system settings
+func (s *SSOSettingsService) mergeSSOSettings(dbSettings, systemSettings *models.SSOSettings) *models.SSOSettings {
+	if dbSettings == nil {
+		s.logger.Debug("No SSO Settings found in the database, using system settings")
+		return systemSettings
+	}
+
+	s.logger.Debug("Merging SSO Settings", "dbSettings", dbSettings.Settings, "systemSettings", systemSettings.Settings)
+
+	finalSettings := mergeSettings(dbSettings.Settings, systemSettings.Settings)
+
+	dbSettings.Settings = finalSettings
+
+	return dbSettings
+}
+
+func mergeSettings(storedSettings, systemSettings map[string]any) map[string]any {
+	settings := make(map[string]any)
+
+	for k, v := range storedSettings {
+		settings[k] = v
+	}
+
+	for k, v := range systemSettings {
+		if _, ok := settings[k]; !ok {
+			settings[k] = v
+		}
+	}
+
+	return settings
+}
+
 func isSecret(fieldName string) bool {
 	secretFieldPatterns := []string{"secret"}
 
@@ -305,22 +316,6 @@ func isSecret(fieldName string) bool {
 	return false
 }
 
-func mergeSettings(apiSettings, systemSettings map[string]any) map[string]any {
-	settings := make(map[string]any)
-
-	for k, v := range apiSettings {
-		settings[k] = v
-	}
-
-	for k, v := range systemSettings {
-		if _, ok := settings[k]; !ok {
-			settings[k] = v
-		}
-	}
-
-	return settings
-}
-
 func isProviderConfigurable(provider string) bool {
 	for _, configurable := range ssosettings.ConfigurableOAuthProviders {
 		if provider == configurable {

pkg/services/ssosettings/ssosettingsimpl/service_test.go

@@ -35,10 +35,21 @@ func TestSSOSettingsService_GetForProvider(t *testing.T) {
 					Settings: map[string]any{"enabled": true},
 					Source:   models.DB,
 				}
+				env.fallbackStrategy.ExpectedIsMatch = true
+				env.fallbackStrategy.ExpectedConfigs = map[string]map[string]any{
+					"github": {
+						"client_id":     "client_id",
+						"client_secret": "secret",
+					},
+				}
 			},
 			want: &models.SSOSettings{
 				Provider: "github",
-				Settings: map[string]any{"enabled": true},
+				Settings: map[string]any{
+					"enabled":       true,
+					"client_id":     "client_id",
+					"client_secret": "secret",
+				},
 			},
 			wantErr: false,
 		},
@@ -49,16 +60,23 @@ func TestSSOSettingsService_GetForProvider(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "should fallback to strategy if store returns not found",
+			name: "should fallback to the system settings if store returns not found",
 			setup: func(env testEnv) {
 				env.store.ExpectedError = ssosettings.ErrNotFound
 				env.fallbackStrategy.ExpectedIsMatch = true
-				env.fallbackStrategy.ExpectedConfig = map[string]any{"enabled": true}
+				env.fallbackStrategy.ExpectedConfigs = map[string]map[string]any{
+					"github": {
+						"enabled":   true,
+						"client_id": "client_id",
+					},
+				}
 			},
 			want: &models.SSOSettings{
 				Provider: "github",
-				Settings: map[string]any{"enabled": true},
-				Source:   models.System,
+				Settings: map[string]any{
+					"enabled":   true,
+					"client_id": "client_id"},
+				Source: models.System,
 			},
 			wantErr: false,
 		},
@@ -146,7 +164,11 @@ func TestSSOSettingsService_GetForProviderWithRedactedSecrets(t *testing.T) {
 			setup: func(env testEnv) {
 				env.store.ExpectedError = ssosettings.ErrNotFound
 				env.fallbackStrategy.ExpectedIsMatch = true
-				env.fallbackStrategy.ExpectedConfig = map[string]any{"enabled": true}
+				env.fallbackStrategy.ExpectedConfigs = map[string]map[string]any{
+					"github": {
+						"enabled": true,
+					},
+				}
 			},
 			want: &models.SSOSettings{
 				Provider: "github",
@@ -219,18 +241,52 @@ func TestSSOSettingsService_List(t *testing.T) {
 					},
 				}
 				env.fallbackStrategy.ExpectedIsMatch = true
-				env.fallbackStrategy.ExpectedConfig = map[string]any{"enabled": false}
+				env.fallbackStrategy.ExpectedConfigs = map[string]map[string]any{
+					"github": {
+						"enabled":       false,
+						"client_id":     "client_id",
+						"client_secret": "client_secret",
+					},
+					"okta": {
+						"enabled":       false,
+						"client_id":     "client_id",
+						"client_secret": "client_secret",
+					},
+					"gitlab": {
+						"enabled": false,
+					},
+					"generic_oauth": {
+						"enabled": false,
+					},
+					"google": {
+						"enabled": false,
+					},
+					"azuread": {
+						"enabled": false,
+					},
+					"grafana_com": {
+						"enabled": false,
+					},
+				}
 			},
 			want: []*models.SSOSettings{
 				{
 					Provider: "github",
-					Settings: map[string]any{"enabled": true},
-					Source:   models.DB,
+					Settings: map[string]any{
+						"enabled":       true,
+						"client_id":     "client_id",
+						"client_secret": "client_secret",
+					},
+					Source: models.DB,
 				},
 				{
 					Provider: "okta",
-					Settings: map[string]any{"enabled": false},
-					Source:   models.DB,
+					Settings: map[string]any{
+						"enabled":       false,
+						"client_id":     "client_id",
+						"client_secret": "client_secret",
+					},
+					Source: models.DB,
 				},
 				{
 					Provider: "gitlab",
@@ -271,7 +327,29 @@ func TestSSOSettingsService_List(t *testing.T) {
 			setup: func(env testEnv) {
 				env.store.ExpectedSSOSettings = []*models.SSOSettings{}
 				env.fallbackStrategy.ExpectedIsMatch = true
-				env.fallbackStrategy.ExpectedConfig = map[string]any{"enabled": false}
+				env.fallbackStrategy.ExpectedConfigs = map[string]map[string]any{
+					"github": {
+						"enabled": false,
+					},
+					"okta": {
+						"enabled": false,
+					},
+					"gitlab": {
+						"enabled": false,
+					},
+					"generic_oauth": {
+						"enabled": false,
+					},
+					"google": {
+						"enabled": false,
+					},
+					"azuread": {
+						"enabled": false,
+					},
+					"grafana_com": {
+						"enabled": false,
+					},
+				}
 			},
 			want: []*models.SSOSettings{
 				{
@@ -370,77 +448,6 @@ func TestSSOSettingsService_Upsert(t *testing.T) {
 		require.EqualValues(t, settings, env.store.ActualSSOSettings)
 	})
 
-	t.Run("successfully upsert SSO settings having system settings", func(t *testing.T) {
-		env := setupTestEnv(t)
-
-		provider := social.GitHubProviderName
-		settings := models.SSOSettings{
-			Provider: provider,
-			Settings: map[string]any{
-				"client_id":     "client-id",
-				"client_secret": "client-secret",
-				"enabled":       true,
-			},
-			IsDeleted: false,
-		}
-		systemSettings := map[string]any{
-			"api_url":           "http://api-url",
-			"use_refresh_token": true,
-		}
-
-		reloadable := ssosettingstests.NewMockReloadable(t)
-		reloadable.On("Validate", mock.Anything, settings).Return(nil)
-		reloadable.On("Reload", mock.Anything, mock.Anything).Return(nil).Maybe()
-		env.reloadables[provider] = reloadable
-		env.fallbackStrategy.ExpectedConfig = systemSettings
-		env.secrets.On("Encrypt", mock.Anything, []byte(settings.Settings["client_secret"].(string)), mock.Anything).Return([]byte("encrypted-client-secret"), nil).Once()
-
-		err := env.service.Upsert(context.Background(), settings)
-		require.NoError(t, err)
-
-		settings.Settings["client_secret"] = "encrypted-client-secret"
-		settings.Settings["api_url"] = systemSettings["api_url"]
-		settings.Settings["use_refresh_token"] = systemSettings["use_refresh_token"]
-		require.EqualValues(t, settings, env.store.ActualSSOSettings)
-	})
-
-	t.Run("successfully upsert SSO settings having system settings without overwriting user settings", func(t *testing.T) {
-		env := setupTestEnv(t)
-
-		provider := social.GitlabProviderName
-		settings := models.SSOSettings{
-			Provider: provider,
-			Settings: map[string]any{
-				"client_id":     "client-id",
-				"client_secret": "client-secret",
-				"enabled":       true,
-			},
-			IsDeleted: false,
-		}
-		systemSettings := map[string]any{
-			"client_id":         "client-id-from-system",
-			"client_secret":     "client-secret-from-system",
-			"enabled":           false,
-			"api_url":           "http://api-url",
-			"use_refresh_token": true,
-		}
-
-		reloadable := ssosettingstests.NewMockReloadable(t)
-		reloadable.On("Validate", mock.Anything, settings).Return(nil)
-		reloadable.On("Reload", mock.Anything, mock.Anything).Return(nil).Maybe()
-		env.reloadables[provider] = reloadable
-		env.fallbackStrategy.ExpectedConfig = systemSettings
-		env.secrets.On("Encrypt", mock.Anything, []byte(settings.Settings["client_secret"].(string)), mock.Anything).Return([]byte("encrypted-client-secret"), nil).Once()
-
-		err := env.service.Upsert(context.Background(), settings)
-		require.NoError(t, err)
-
-		settings.Settings["client_secret"] = "encrypted-client-secret"
-		settings.Settings["api_url"] = systemSettings["api_url"]
-		settings.Settings["use_refresh_token"] = systemSettings["use_refresh_token"]
-		require.EqualValues(t, settings, env.store.ActualSSOSettings)
-	})
-
 	t.Run("returns error if provider is not configurable", func(t *testing.T) {
 		env := setupTestEnv(t)
 
@@ -697,7 +704,7 @@ func setupTestEnv(t *testing.T) testEnv {
 	fallbackStrategy.ExpectedIsMatch = true
 
 	svc := &SSOSettingsService{
-		log:          log.NewNopLogger(),
+		logger:       log.NewNopLogger(),
 		store:        store,
 		ac:           accessControl,
 		fbStrategies: []ssosettings.FallbackStrategy{fallbackStrategy},

pkg/services/ssosettings/ssosettingstests/fallback_strategy_fake.go

@@ -4,7 +4,7 @@ import context "context"
 
 type FakeFallbackStrategy struct {
 	ExpectedIsMatch bool
-	ExpectedConfig  map[string]any
+	ExpectedConfigs map[string]map[string]any
 
 	ExpectedError error
 }
@@ -18,5 +18,5 @@ func (f *FakeFallbackStrategy) IsMatch(provider string) bool {
 }
 
 func (f *FakeFallbackStrategy) GetProviderConfig(ctx context.Context, provider string) (map[string]any, error) {
-	return f.ExpectedConfig, f.ExpectedError
+	return f.ExpectedConfigs[provider], f.ExpectedError
 }