From e1aedb65b395401dd86b2d5b27d32bb0fad8df28 Mon Sep 17 00:00:00 2001 From: Mihai Doarna Date: Fri, 31 May 2024 11:08:52 +0300 Subject: [PATCH] SSO: Add oldSettings param to the Validate function from SSO settings (#88245) * add oldSettings param to the Validate function from SSO settings * update unit tests adding the missing param to Validate --- pkg/login/social/connectors/azuread_oauth.go | 2 +- pkg/login/social/connectors/azuread_oauth_test.go | 2 +- pkg/login/social/connectors/generic_oauth.go | 2 +- pkg/login/social/connectors/generic_oauth_test.go | 2 +- pkg/login/social/connectors/github_oauth.go | 2 +- pkg/login/social/connectors/github_oauth_test.go | 2 +- pkg/login/social/connectors/gitlab_oauth.go | 2 +- pkg/login/social/connectors/gitlab_oauth_test.go | 2 +- pkg/login/social/connectors/google_oauth.go | 2 +- pkg/login/social/connectors/google_oauth_test.go | 2 +- pkg/login/social/connectors/grafana_com_oauth.go | 2 +- .../social/connectors/grafana_com_oauth_test.go | 2 +- pkg/login/social/connectors/okta_oauth.go | 2 +- pkg/login/social/connectors/okta_oauth_test.go | 2 +- pkg/services/ssosettings/ssosettings.go | 2 +- .../ssosettings/ssosettingsimpl/service.go | 2 +- .../ssosettings/ssosettingsimpl/service_test.go | 14 +++++++------- .../ssosettingstests/reloadable_mock.go | 12 ++++++------ 18 files changed, 29 insertions(+), 29 deletions(-) diff --git a/pkg/login/social/connectors/azuread_oauth.go b/pkg/login/social/connectors/azuread_oauth.go index 2eb89778cb6..32876aa7a5c 100644 --- a/pkg/login/social/connectors/azuread_oauth.go +++ b/pkg/login/social/connectors/azuread_oauth.go @@ -191,7 +191,7 @@ func (s *SocialAzureAD) Reload(ctx context.Context, settings ssoModels.SSOSettin return nil } -func (s *SocialAzureAD) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { +func (s *SocialAzureAD) Validate(ctx context.Context, settings ssoModels.SSOSettings, _ ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) diff --git a/pkg/login/social/connectors/azuread_oauth_test.go b/pkg/login/social/connectors/azuread_oauth_test.go index 05b6a90d680..92aa7f67c10 100644 --- a/pkg/login/social/connectors/azuread_oauth_test.go +++ b/pkg/login/social/connectors/azuread_oauth_test.go @@ -1130,7 +1130,7 @@ func TestSocialAzureAD_Validate(t *testing.T) { if tc.requester == nil { tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} } - err := s.Validate(context.Background(), tc.settings, tc.requester) + err := s.Validate(context.Background(), tc.settings, ssoModels.SSOSettings{}, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/login/social/connectors/generic_oauth.go b/pkg/login/social/connectors/generic_oauth.go index 6b4ba98563c..24feb363c0a 100644 --- a/pkg/login/social/connectors/generic_oauth.go +++ b/pkg/login/social/connectors/generic_oauth.go @@ -75,7 +75,7 @@ func NewGenericOAuthProvider(info *social.OAuthInfo, cfg *setting.Cfg, orgRoleMa return provider } -func (s *SocialGenericOAuth) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { +func (s *SocialGenericOAuth) Validate(ctx context.Context, settings ssoModels.SSOSettings, _ ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) diff --git a/pkg/login/social/connectors/generic_oauth_test.go b/pkg/login/social/connectors/generic_oauth_test.go index a094b1fe79e..5d764ef7541 100644 --- a/pkg/login/social/connectors/generic_oauth_test.go +++ b/pkg/login/social/connectors/generic_oauth_test.go @@ -1090,7 +1090,7 @@ func TestSocialGenericOAuth_Validate(t *testing.T) { if tc.requester == nil { tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} } - err := s.Validate(context.Background(), tc.settings, tc.requester) + err := s.Validate(context.Background(), tc.settings, ssoModels.SSOSettings{}, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/login/social/connectors/github_oauth.go b/pkg/login/social/connectors/github_oauth.go index 85022c6f030..73f0c90683a 100644 --- a/pkg/login/social/connectors/github_oauth.go +++ b/pkg/login/social/connectors/github_oauth.go @@ -82,7 +82,7 @@ func NewGitHubProvider(info *social.OAuthInfo, cfg *setting.Cfg, orgRoleMapper * return provider } -func (s *SocialGithub) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { +func (s *SocialGithub) Validate(ctx context.Context, settings ssoModels.SSOSettings, _ ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) diff --git a/pkg/login/social/connectors/github_oauth_test.go b/pkg/login/social/connectors/github_oauth_test.go index ff8ff1b4c62..c66f7596cf9 100644 --- a/pkg/login/social/connectors/github_oauth_test.go +++ b/pkg/login/social/connectors/github_oauth_test.go @@ -501,7 +501,7 @@ func TestSocialGitHub_Validate(t *testing.T) { tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} } - err := s.Validate(context.Background(), tc.settings, tc.requester) + err := s.Validate(context.Background(), tc.settings, ssoModels.SSOSettings{}, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/login/social/connectors/gitlab_oauth.go b/pkg/login/social/connectors/gitlab_oauth.go index 8e1018dd80f..4d94e0a1f8e 100644 --- a/pkg/login/social/connectors/gitlab_oauth.go +++ b/pkg/login/social/connectors/gitlab_oauth.go @@ -66,7 +66,7 @@ func NewGitLabProvider(info *social.OAuthInfo, cfg *setting.Cfg, orgRoleMapper * return provider } -func (s *SocialGitlab) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { +func (s *SocialGitlab) Validate(ctx context.Context, settings ssoModels.SSOSettings, _ ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) diff --git a/pkg/login/social/connectors/gitlab_oauth_test.go b/pkg/login/social/connectors/gitlab_oauth_test.go index e61bcc55717..75b0f339bdc 100644 --- a/pkg/login/social/connectors/gitlab_oauth_test.go +++ b/pkg/login/social/connectors/gitlab_oauth_test.go @@ -582,7 +582,7 @@ func TestSocialGitlab_Validate(t *testing.T) { tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} } - err := s.Validate(context.Background(), tc.settings, tc.requester) + err := s.Validate(context.Background(), tc.settings, ssoModels.SSOSettings{}, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/login/social/connectors/google_oauth.go b/pkg/login/social/connectors/google_oauth.go index e9dc00d924f..a8da90b9edc 100644 --- a/pkg/login/social/connectors/google_oauth.go +++ b/pkg/login/social/connectors/google_oauth.go @@ -65,7 +65,7 @@ func NewGoogleProvider(info *social.OAuthInfo, cfg *setting.Cfg, orgRoleMapper * return provider } -func (s *SocialGoogle) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { +func (s *SocialGoogle) Validate(ctx context.Context, settings ssoModels.SSOSettings, _ ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) diff --git a/pkg/login/social/connectors/google_oauth_test.go b/pkg/login/social/connectors/google_oauth_test.go index d63ecca6c6d..971d58ba7bd 100644 --- a/pkg/login/social/connectors/google_oauth_test.go +++ b/pkg/login/social/connectors/google_oauth_test.go @@ -803,7 +803,7 @@ func TestSocialGoogle_Validate(t *testing.T) { tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} } - err := s.Validate(context.Background(), tc.settings, tc.requester) + err := s.Validate(context.Background(), tc.settings, ssoModels.SSOSettings{}, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/login/social/connectors/grafana_com_oauth.go b/pkg/login/social/connectors/grafana_com_oauth.go index 837c7572ae6..51964891823 100644 --- a/pkg/login/social/connectors/grafana_com_oauth.go +++ b/pkg/login/social/connectors/grafana_com_oauth.go @@ -56,7 +56,7 @@ func NewGrafanaComProvider(info *social.OAuthInfo, cfg *setting.Cfg, orgRoleMapp return provider } -func (s *SocialGrafanaCom) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { +func (s *SocialGrafanaCom) Validate(ctx context.Context, settings ssoModels.SSOSettings, _ ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) diff --git a/pkg/login/social/connectors/grafana_com_oauth_test.go b/pkg/login/social/connectors/grafana_com_oauth_test.go index 41d44b08a8a..b6b082ba43a 100644 --- a/pkg/login/social/connectors/grafana_com_oauth_test.go +++ b/pkg/login/social/connectors/grafana_com_oauth_test.go @@ -205,7 +205,7 @@ func TestSocialGrafanaCom_Validate(t *testing.T) { tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} } - err := s.Validate(context.Background(), tc.settings, tc.requester) + err := s.Validate(context.Background(), tc.settings, ssoModels.SSOSettings{}, tc.requester) if tc.expectError { require.Error(t, err) } else { diff --git a/pkg/login/social/connectors/okta_oauth.go b/pkg/login/social/connectors/okta_oauth.go index 381c3bbd324..3dfa3ca6b21 100644 --- a/pkg/login/social/connectors/okta_oauth.go +++ b/pkg/login/social/connectors/okta_oauth.go @@ -62,7 +62,7 @@ func NewOktaProvider(info *social.OAuthInfo, cfg *setting.Cfg, orgRoleMapper *Or return provider } -func (s *SocialOkta) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { +func (s *SocialOkta) Validate(ctx context.Context, settings ssoModels.SSOSettings, _ ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) diff --git a/pkg/login/social/connectors/okta_oauth_test.go b/pkg/login/social/connectors/okta_oauth_test.go index 74a3df3b8f2..cabc1b349a7 100644 --- a/pkg/login/social/connectors/okta_oauth_test.go +++ b/pkg/login/social/connectors/okta_oauth_test.go @@ -287,7 +287,7 @@ func TestSocialOkta_Validate(t *testing.T) { if tc.requester == nil { tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} } - err := s.Validate(context.Background(), tc.settings, tc.requester) + err := s.Validate(context.Background(), tc.settings, ssoModels.SSOSettings{}, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/services/ssosettings/ssosettings.go b/pkg/services/ssosettings/ssosettings.go index 56e79e58bfe..764c7b88efb 100644 --- a/pkg/services/ssosettings/ssosettings.go +++ b/pkg/services/ssosettings/ssosettings.go @@ -41,7 +41,7 @@ type Service interface { //go:generate mockery --name Reloadable --structname MockReloadable --outpkg ssosettingstests --filename reloadable_mock.go --output ./ssosettingstests/ type Reloadable interface { Reload(ctx context.Context, settings models.SSOSettings) error - Validate(ctx context.Context, settings models.SSOSettings, requester identity.Requester) error + Validate(ctx context.Context, settings models.SSOSettings, oldSettings models.SSOSettings, requester identity.Requester) error } // FallbackStrategy is an interface that can be implemented to allow a provider to load settings from a different source diff --git a/pkg/services/ssosettings/ssosettingsimpl/service.go b/pkg/services/ssosettings/ssosettingsimpl/service.go index 073204bb3ce..9ada70288a9 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service.go @@ -200,7 +200,7 @@ func (s *Service) Upsert(ctx context.Context, settings *models.SSOSettings, requ } settings.Settings = settingsWithSecrets - err = reloadable.Validate(ctx, *settings, requester) + err = reloadable.Validate(ctx, *settings, *storedSettings, requester) if err != nil { return err } diff --git a/pkg/services/ssosettings/ssosettingsimpl/service_test.go b/pkg/services/ssosettings/ssosettingsimpl/service_test.go index b83db923dd1..97376888580 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service_test.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service_test.go @@ -892,7 +892,7 @@ func TestService_Upsert(t *testing.T) { wg.Add(1) reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(nil) + reloadable.On("Validate", mock.Anything, settings, mock.Anything, mock.Anything).Return(nil) reloadable.On("Reload", mock.Anything, mock.MatchedBy(func(settings models.SSOSettings) bool { defer wg.Done() return settings.Provider == provider && @@ -1000,7 +1000,7 @@ func TestService_Upsert(t *testing.T) { } reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(errors.New("validation failed")) + reloadable.On("Validate", mock.Anything, settings, mock.Anything, mock.Anything).Return(errors.New("validation failed")) env.reloadables[provider] = reloadable err := env.service.Upsert(context.Background(), &settings, &user.SignedInUser{}) @@ -1068,7 +1068,7 @@ func TestService_Upsert(t *testing.T) { } reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(nil) + reloadable.On("Validate", mock.Anything, settings, mock.Anything, mock.Anything).Return(nil) env.reloadables[provider] = reloadable env.secrets.On("Encrypt", mock.Anything, []byte(settings.Settings["client_secret"].(string)), mock.Anything).Return(nil, errors.New("encryption failed")).Once() @@ -1107,7 +1107,7 @@ func TestService_Upsert(t *testing.T) { expected.Settings["client_secret"] = "encrypted-client-secret" reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, expected, mock.Anything).Return(nil) + reloadable.On("Validate", mock.Anything, expected, mock.Anything, mock.Anything).Return(nil) reloadable.On("Reload", mock.Anything, mock.Anything).Return(nil).Maybe() env.reloadables[provider] = reloadable env.secrets.On("Decrypt", mock.Anything, []byte("current-client-secret"), mock.Anything).Return([]byte("encrypted-client-secret"), nil).Once() @@ -1154,7 +1154,7 @@ func TestService_Upsert(t *testing.T) { expected.Settings["private_key"] = "current-private-key" reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, expected, mock.Anything).Return(nil) + reloadable.On("Validate", mock.Anything, expected, mock.Anything, mock.Anything).Return(nil) reloadable.On("Reload", mock.Anything, mock.Anything).Return(nil).Maybe() env.reloadables[provider] = reloadable env.secrets.On("Decrypt", mock.Anything, []byte("encrypted-current-client-secret"), mock.Anything).Return([]byte("current-client-secret"), nil).Once() @@ -1190,7 +1190,7 @@ func TestService_Upsert(t *testing.T) { } reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(nil) + reloadable.On("Validate", mock.Anything, settings, mock.Anything, mock.Anything).Return(nil) env.reloadables[provider] = reloadable env.secrets.On("Encrypt", mock.Anything, []byte(settings.Settings["client_secret"].(string)), mock.Anything).Return([]byte("encrypted-client-secret"), nil).Once() env.store.GetFn = func(ctx context.Context, provider string) (*models.SSOSettings, error) { @@ -1222,7 +1222,7 @@ func TestService_Upsert(t *testing.T) { } reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(nil) + reloadable.On("Validate", mock.Anything, settings, mock.Anything, mock.Anything).Return(nil) reloadable.On("Reload", mock.Anything, mock.Anything).Return(errors.New("failed reloading new settings")).Maybe() env.reloadables[provider] = reloadable env.secrets.On("Encrypt", mock.Anything, []byte(settings.Settings["client_secret"].(string)), mock.Anything).Return([]byte("encrypted-client-secret"), nil).Once() diff --git a/pkg/services/ssosettings/ssosettingstests/reloadable_mock.go b/pkg/services/ssosettings/ssosettingstests/reloadable_mock.go index 599fe1c5c19..f6bc845d7aa 100644 --- a/pkg/services/ssosettings/ssosettingstests/reloadable_mock.go +++ b/pkg/services/ssosettings/ssosettingstests/reloadable_mock.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.40.1. DO NOT EDIT. +// Code generated by mockery v2.42.1. DO NOT EDIT. package ssosettingstests @@ -34,17 +34,17 @@ func (_m *MockReloadable) Reload(ctx context.Context, settings models.SSOSetting return r0 } -// Validate provides a mock function with given fields: ctx, settings, requester -func (_m *MockReloadable) Validate(ctx context.Context, settings models.SSOSettings, requester identity.Requester) error { - ret := _m.Called(ctx, settings, requester) +// Validate provides a mock function with given fields: ctx, settings, oldSettings, requester +func (_m *MockReloadable) Validate(ctx context.Context, settings models.SSOSettings, oldSettings models.SSOSettings, requester identity.Requester) error { + ret := _m.Called(ctx, settings, oldSettings, requester) if len(ret) == 0 { panic("no return value specified for Validate") } var r0 error - if rf, ok := ret.Get(0).(func(context.Context, models.SSOSettings, identity.Requester) error); ok { - r0 = rf(ctx, settings, requester) + if rf, ok := ret.Get(0).(func(context.Context, models.SSOSettings, models.SSOSettings, identity.Requester) error); ok { + r0 = rf(ctx, settings, oldSettings, requester) } else { r0 = ret.Error(0) }