diff --git a/pkg/services/ssosettings/ssosettingsimpl/service.go b/pkg/services/ssosettings/ssosettingsimpl/service.go index d3cc7d12568..2b28a43a482 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service.go +++ b/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,42 +252,47 @@ 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 } } } -func isSecret(fieldName string) bool { - secretFieldPatterns := []string{"secret"} - - for _, v := range secretFieldPatterns { - if strings.Contains(strings.ToLower(fieldName), strings.ToLower(v)) { - return true - } +// 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 } - return false + + 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(apiSettings, systemSettings map[string]any) map[string]any { +func mergeSettings(storedSettings, systemSettings map[string]any) map[string]any { settings := make(map[string]any) - for k, v := range apiSettings { + for k, v := range storedSettings { settings[k] = v } @@ -321,6 +305,17 @@ func mergeSettings(apiSettings, systemSettings map[string]any) map[string]any { return settings } +func isSecret(fieldName string) bool { + secretFieldPatterns := []string{"secret"} + + for _, v := range secretFieldPatterns { + if strings.Contains(strings.ToLower(fieldName), strings.ToLower(v)) { + return true + } + } + return false +} + func isProviderConfigurable(provider string) bool { for _, configurable := range ssosettings.ConfigurableOAuthProviders { if provider == configurable { diff --git a/pkg/services/ssosettings/ssosettingsimpl/service_test.go b/pkg/services/ssosettings/ssosettingsimpl/service_test.go index 104eb200a92..d146c20d98a 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service_test.go +++ b/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}, diff --git a/pkg/services/ssosettings/ssosettingstests/fallback_strategy_fake.go b/pkg/services/ssosettings/ssosettingstests/fallback_strategy_fake.go index 0a8ae3a9d39..ed514f9a990 100644 --- a/pkg/services/ssosettings/ssosettingstests/fallback_strategy_fake.go +++ b/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 } diff --git a/public/app/features/auth-config/ProviderConfigForm.test.tsx b/public/app/features/auth-config/ProviderConfigForm.test.tsx index f90568d0d55..930751520b1 100644 --- a/public/app/features/auth-config/ProviderConfigForm.test.tsx +++ b/public/app/features/auth-config/ProviderConfigForm.test.tsx @@ -93,15 +93,11 @@ describe('ProviderConfigForm', () => { expect(putMock).toHaveBeenCalledWith('/api/v1/sso-settings/github', { ...testConfig, settings: { - ...testConfig.settings, allowedOrganizations: 'test-org1,test-org2', clientId: 'test-client-id', clientSecret: 'test-client-secret', teamIds: '12324', enabled: true, - allowedDomains: '', - allowedGroups: '', - scopes: '', }, }); }); diff --git a/public/app/features/auth-config/ProviderConfigForm.tsx b/public/app/features/auth-config/ProviderConfigForm.tsx index 679e0d640b8..3734ff36a05 100644 --- a/public/app/features/auth-config/ProviderConfigForm.tsx +++ b/public/app/features/auth-config/ProviderConfigForm.tsx @@ -42,12 +42,12 @@ export const ProviderConfigForm = ({ config, provider, isLoading }: ProviderConf const onSubmit = async (data: SSOProviderDTO) => { setIsSaving(true); setSubmitError(false); - const requestData = dtoToData(data); + const requestData = dtoToData(data, provider); try { await getBackendSrv().put(`/api/v1/sso-settings/${provider}`, { id: config?.id, provider: config?.provider, - settings: { ...config?.settings, ...requestData }, + settings: { ...requestData }, }); appEvents.publish({ diff --git a/public/app/features/auth-config/utils/data.ts b/public/app/features/auth-config/utils/data.ts index 472161bde49..55fdbd0fdb1 100644 --- a/public/app/features/auth-config/utils/data.ts +++ b/public/app/features/auth-config/utils/data.ts @@ -1,6 +1,6 @@ import { SelectableValue } from '@grafana/data'; -import { fieldMap } from '../fields'; +import { fieldMap, fields } from '../fields'; import { FieldData, SSOProvider, SSOProviderDTO } from '../types'; import { isSelectableValue } from './guards'; @@ -72,13 +72,29 @@ const valuesToString = (values: Array>) => { return values.map(({ value }) => value).join(','); }; +const includeRequiredKeysOnly = ( + obj: SSOProviderDTO, + requiredKeys: Array +): Partial => { + if (!requiredKeys) { + return obj; + } + let result: Partial = {}; + for (const key of requiredKeys) { + //@ts-expect-error + result[key] = obj[key]; + } + return result; +}; + // Convert the DTO to the data format used by the API -export function dtoToData(dto: SSOProviderDTO) { +export function dtoToData(dto: SSOProviderDTO, provider: string) { const arrayFields = getArrayFields(fieldMap); - const settings = { ...dto }; + const dtoWithRequiredFields = includeRequiredKeysOnly(dto, [...fields[provider], 'enabled']); + const settings = { ...dtoWithRequiredFields }; for (const field of arrayFields) { - const value = dto[field]; + const value = dtoWithRequiredFields[field]; if (value) { if (isSelectableValue(value)) { //@ts-expect-error