SSO SAML: Remove SettingProvider settings from SSO interactions (#94900)

* Remove SettingProvider settings from SSO interactions

* Mock Settings Provider for SSO Settings test

* Ignore error from SettingsProvider

* Add test for backend

pkg/services/ssosettings/ssosettingsimpl/service.go

@@ -31,12 +31,13 @@ import (
 var _ ssosettings.Service = (*Service)(nil)
 
 type Service struct {
-	logger  log.Logger
+	logger           log.Logger
-	cfg     *setting.Cfg
+	cfg              *setting.Cfg
-	store   ssosettings.Store
+	store            ssosettings.Store
-	ac      ac.AccessControl
+	settingsProvider setting.Provider
-	secrets secrets.Service
+	ac               ac.AccessControl
-	metrics *metrics
+	secrets          secrets.Service
+	metrics          *metrics
 
 	fbStrategies          []ssosettings.FallbackStrategy
 	providersList         []string
@@ -87,6 +88,7 @@ func ProvideService(cfg *setting.Cfg, sqlStore db.DB, ac ac.AccessControl,
 		providersList:         providersList,
 		configurableProviders: configurableProviders,
 		reloadables:           make(map[string]ssosettings.Reloadable),
+		settingsProvider:      settingsProvider,
 	}
 
 	usageStats.RegisterMetricsFunc(svc.getUsageStats)
@@ -240,7 +242,7 @@ func (s *Service) Delete(ctx context.Context, provider string) error {
 		return ssosettings.ErrNotConfigurable
 	}
 
-	social, ok := s.reloadables[provider]
+	reloadable, ok := s.reloadables[provider]
 	if !ok {
 		return ssosettings.ErrInvalidProvider.Errorf("provider %s not found in reloadables", provider)
 	}
@@ -250,13 +252,26 @@ func (s *Service) Delete(ctx context.Context, provider string) error {
 		return err
 	}
 
+	// When deleting settings for SAML, clear the Settings table
+	if provider == social.SAMLProviderName {
+		samlSettings := setting.SettingsRemovals{
+			"auth.saml": make([]string, 0, len(s.settingsProvider.Current())),
+		}
+		for k := range s.settingsProvider.Current()["auth.saml"] {
+			samlSettings["auth.saml"] = append(samlSettings["auth.saml"], k)
+		}
+		if err := s.settingsProvider.Update(setting.SettingsBag{}, samlSettings); err != nil {
+			s.logger.Warn("Failed to remove SAML settings from the settings table", "error", err)
+		}
+	}
+
 	currentSettings, err := s.GetForProvider(ctx, provider)
 	if err != nil {
 		s.logger.Error("failed to get current settings, skipping reload", "provider", provider, "error", err)
 		return nil
 	}
 
-	go s.reload(social, provider, *currentSettings)
+	go s.reload(reloadable, provider, *currentSettings)
 
 	return nil
 }

pkg/services/ssosettings/ssosettingsimpl/service_test.go

@@ -30,6 +30,7 @@ import (
 	"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/setting/settingtest"
 	"github.com/grafana/grafana/pkg/tests/testsuite"
 )
 
@@ -1520,6 +1521,38 @@ func TestService_Delete(t *testing.T) {
 
 		require.NoError(t, err)
 	})
+
+	t.Run("should delete SAML SettingsProvider while deleting SAML SSO Settings", func(t *testing.T) {
+		t.Parallel()
+		env := setupTestEnv(t, true, true, true, false)
+
+		mockProvider := &settingtest.MockProvider{}
+		mockProvider.On("Current", mock.Anything).Return(setting.SettingsBag{
+			"auth.saml": map[string]string{
+				"name": "mockedName",
+			},
+		}).Twice()
+		mockProvider.On(
+			"Update",
+			setting.SettingsBag{},
+			setting.SettingsRemovals{"auth.saml": []string{"name"}}).Return(nil).Once()
+		env.service.settingsProvider = mockProvider
+
+		provider := social.SAMLProviderName
+		reloadable := ssosettingstests.NewMockReloadable(t)
+
+		var wg sync.WaitGroup
+		wg.Add(1)
+		reloadable.On("Reload", mock.Anything, mock.MatchedBy(func(settings models.SSOSettings) bool {
+			wg.Done()
+			return settings.Provider == provider && settings.ID == ""
+		})).Return(nil).Once()
+		env.reloadables[provider] = reloadable
+
+		err := env.service.Delete(context.Background(), provider)
+		require.NoError(t, err)
+		wg.Wait()
+	})
 }
 
 // we might not need this test because it is not testing the public interface

pkg/setting/provider.go

@@ -31,6 +31,8 @@ func (v ValidationError) Error() string {
 
 // Provider is a settings provider abstraction
 // with thread-safety and runtime updates.
+//
+//go:generate mockery --name Provider --structname MockProvider --outpkg settingtest --filename provider_mock.go --output ./settingtest/
 type Provider interface {
 	// Current returns a SettingsBag with a static copy of
 	// the current configured pairs of key/values for each

pkg/setting/settingtest/provider_mock.go

@@ -0,0 +1,130 @@
+// Code generated by mockery v2.46.3. DO NOT EDIT.
+
+package settingtest
+
+import (
+	setting "github.com/grafana/grafana/pkg/setting"
+	mock "github.com/stretchr/testify/mock"
+)
+
+// MockProvider is an autogenerated mock type for the Provider type
+type MockProvider struct {
+	mock.Mock
+}
+
+// Current provides a mock function with given fields:
+func (_m *MockProvider) Current() setting.SettingsBag {
+	ret := _m.Called()
+
+	if len(ret) == 0 {
+		panic("no return value specified for Current")
+	}
+
+	var r0 setting.SettingsBag
+	if rf, ok := ret.Get(0).(func() setting.SettingsBag); ok {
+		r0 = rf()
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(setting.SettingsBag)
+		}
+	}
+
+	return r0
+}
+
+// CurrentVerbose provides a mock function with given fields:
+func (_m *MockProvider) CurrentVerbose() setting.VerboseSettingsBag {
+	ret := _m.Called()
+
+	if len(ret) == 0 {
+		panic("no return value specified for CurrentVerbose")
+	}
+
+	var r0 setting.VerboseSettingsBag
+	if rf, ok := ret.Get(0).(func() setting.VerboseSettingsBag); ok {
+		r0 = rf()
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(setting.VerboseSettingsBag)
+		}
+	}
+
+	return r0
+}
+
+// KeyValue provides a mock function with given fields: section, key
+func (_m *MockProvider) KeyValue(section string, key string) setting.KeyValue {
+	ret := _m.Called(section, key)
+
+	if len(ret) == 0 {
+		panic("no return value specified for KeyValue")
+	}
+
+	var r0 setting.KeyValue
+	if rf, ok := ret.Get(0).(func(string, string) setting.KeyValue); ok {
+		r0 = rf(section, key)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(setting.KeyValue)
+		}
+	}
+
+	return r0
+}
+
+// RegisterReloadHandler provides a mock function with given fields: section, handler
+func (_m *MockProvider) RegisterReloadHandler(section string, handler setting.ReloadHandler) {
+	_m.Called(section, handler)
+}
+
+// Section provides a mock function with given fields: section
+func (_m *MockProvider) Section(section string) setting.Section {
+	ret := _m.Called(section)
+
+	if len(ret) == 0 {
+		panic("no return value specified for Section")
+	}
+
+	var r0 setting.Section
+	if rf, ok := ret.Get(0).(func(string) setting.Section); ok {
+		r0 = rf(section)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(setting.Section)
+		}
+	}
+
+	return r0
+}
+
+// Update provides a mock function with given fields: updates, removals
+func (_m *MockProvider) Update(updates setting.SettingsBag, removals setting.SettingsRemovals) error {
+	ret := _m.Called(updates, removals)
+
+	if len(ret) == 0 {
+		panic("no return value specified for Update")
+	}
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(setting.SettingsBag, setting.SettingsRemovals) error); ok {
+		r0 = rf(updates, removals)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// NewMockProvider creates a new instance of MockProvider. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations.
+// The first argument is typically a *testing.T value.
+func NewMockProvider(t interface {
+	mock.TestingT
+	Cleanup(func())
+}) *MockProvider {
+	mock := &MockProvider{}
+	mock.Mock.Test(t)
+
+	t.Cleanup(func() { mock.AssertExpectations(t) })
+
+	return mock
+}