From 640bc0de974a731fe5b515c1f8662458a537ae23 Mon Sep 17 00:00:00 2001 From: linoman <2051016+linoman@users.noreply.github.com> Date: Mon, 21 Oct 2024 11:28:55 +0200 Subject: [PATCH] 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 --- .../ssosettings/ssosettingsimpl/service.go | 31 +++-- .../ssosettingsimpl/service_test.go | 33 +++++ pkg/setting/provider.go | 2 + pkg/setting/settingtest/provider_mock.go | 130 ++++++++++++++++++ 4 files changed, 188 insertions(+), 8 deletions(-) create mode 100644 pkg/setting/settingtest/provider_mock.go diff --git a/pkg/services/ssosettings/ssosettingsimpl/service.go b/pkg/services/ssosettings/ssosettingsimpl/service.go index 5307ac21cfe..60ae72c9072 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service.go @@ -31,12 +31,13 @@ import ( var _ ssosettings.Service = (*Service)(nil) type Service struct { - logger log.Logger - cfg *setting.Cfg - store ssosettings.Store - ac ac.AccessControl - secrets secrets.Service - metrics *metrics + logger log.Logger + cfg *setting.Cfg + store ssosettings.Store + settingsProvider setting.Provider + ac ac.AccessControl + 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 } diff --git a/pkg/services/ssosettings/ssosettingsimpl/service_test.go b/pkg/services/ssosettings/ssosettingsimpl/service_test.go index cabf5dfe719..fef330771e1 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service_test.go +++ b/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 diff --git a/pkg/setting/provider.go b/pkg/setting/provider.go index 1978689f40b..7ac9a7cb3ca 100644 --- a/pkg/setting/provider.go +++ b/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 diff --git a/pkg/setting/settingtest/provider_mock.go b/pkg/setting/settingtest/provider_mock.go new file mode 100644 index 00000000000..9cccb382d72 --- /dev/null +++ b/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 +}