From 6465d87afdf4b29a34332cf78b2f0c1aa2191812 Mon Sep 17 00:00:00 2001 From: Mihai Doarna Date: Wed, 3 Jan 2024 10:02:03 +0200 Subject: [PATCH] Auth: Add basic validation for SSO settings (#79696) * add basic validation for sso settings * remove validation for the client secret --- pkg/login/social/connectors/azuread_oauth.go | 12 ++++ .../social/connectors/azuread_oauth_test.go | 58 +++++++++++++++++++ pkg/login/social/connectors/generic_oauth.go | 12 ++++ .../social/connectors/generic_oauth_test.go | 58 +++++++++++++++++++ pkg/login/social/connectors/github_oauth.go | 12 ++++ .../social/connectors/github_oauth_test.go | 58 +++++++++++++++++++ pkg/login/social/connectors/gitlab_oauth.go | 12 ++++ .../social/connectors/gitlab_oauth_test.go | 58 +++++++++++++++++++ pkg/login/social/connectors/google_oauth.go | 12 ++++ .../social/connectors/google_oauth_test.go | 58 +++++++++++++++++++ .../social/connectors/grafana_com_oauth.go | 12 ++++ .../connectors/grafana_com_oauth_test.go | 58 +++++++++++++++++++ pkg/login/social/connectors/okta_oauth.go | 12 ++++ .../social/connectors/okta_oauth_test.go | 58 +++++++++++++++++++ pkg/login/social/connectors/social_base.go | 9 +++ pkg/services/ssosettings/errors.go | 9 ++- 16 files changed, 507 insertions(+), 1 deletion(-) diff --git a/pkg/login/social/connectors/azuread_oauth.go b/pkg/login/social/connectors/azuread_oauth.go index a3ad4f3b858..feba846069a 100644 --- a/pkg/login/social/connectors/azuread_oauth.go +++ b/pkg/login/social/connectors/azuread_oauth.go @@ -163,6 +163,18 @@ func (s *SocialAzureAD) UserInfo(ctx context.Context, client *http.Client, token } func (s *SocialAzureAD) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { + info, err := CreateOAuthInfoFromKeyValues(settings.Settings) + if err != nil { + return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) + } + + err = validateInfo(info) + if err != nil { + return err + } + + // add specific validation rules for AzureAD + return nil } diff --git a/pkg/login/social/connectors/azuread_oauth_test.go b/pkg/login/social/connectors/azuread_oauth_test.go index f974d132a4b..2e4a9153731 100644 --- a/pkg/login/social/connectors/azuread_oauth_test.go +++ b/pkg/login/social/connectors/azuread_oauth_test.go @@ -19,6 +19,7 @@ import ( "github.com/grafana/grafana/pkg/infra/remotecache" "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/services/featuremgmt" + ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" "github.com/grafana/grafana/pkg/setting" ) @@ -987,3 +988,60 @@ func TestSocialAzureAD_InitializeExtraFields(t *testing.T) { }) } } + +func TestSocialAzureAD_Validate(t *testing.T) { + testCases := []struct { + name string + settings ssoModels.SSOSettings + expectError bool + }{ + { + name: "SSOSettings is valid", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + }, + }, + expectError: false, + }, + { + name: "fails if settings map contains an invalid field", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "invalid_field": []int{1, 2, 3}, + }, + }, + expectError: true, + }, + { + name: "fails if client id is empty", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "", + }, + }, + expectError: true, + }, + { + name: "fails if client id does not exist", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{}, + }, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + s := NewAzureADProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures(), nil) + + err := s.Validate(context.Background(), tc.settings) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/login/social/connectors/generic_oauth.go b/pkg/login/social/connectors/generic_oauth.go index 6728c54f52a..156ec78910f 100644 --- a/pkg/login/social/connectors/generic_oauth.go +++ b/pkg/login/social/connectors/generic_oauth.go @@ -69,6 +69,18 @@ func NewGenericOAuthProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettin } func (s *SocialGenericOAuth) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { + info, err := CreateOAuthInfoFromKeyValues(settings.Settings) + if err != nil { + return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) + } + + err = validateInfo(info) + if err != nil { + return err + } + + // add specific validation rules for Generic OAuth + return nil } diff --git a/pkg/login/social/connectors/generic_oauth_test.go b/pkg/login/social/connectors/generic_oauth_test.go index dbb5d482c06..655f88875cc 100644 --- a/pkg/login/social/connectors/generic_oauth_test.go +++ b/pkg/login/social/connectors/generic_oauth_test.go @@ -15,6 +15,7 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" + ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" "github.com/grafana/grafana/pkg/setting" ) @@ -915,3 +916,60 @@ func TestSocialGenericOAuth_InitializeExtraFields(t *testing.T) { }) } } + +func TestSocialGenericOAuth_Validate(t *testing.T) { + testCases := []struct { + name string + settings ssoModels.SSOSettings + expectError bool + }{ + { + name: "SSOSettings is valid", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + }, + }, + expectError: false, + }, + { + name: "fails if settings map contains an invalid field", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "invalid_field": []int{1, 2, 3}, + }, + }, + expectError: true, + }, + { + name: "fails if client id is empty", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "", + }, + }, + expectError: true, + }, + { + name: "fails if client id does not exist", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{}, + }, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + s := NewGenericOAuthProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) + + err := s.Validate(context.Background(), tc.settings) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/login/social/connectors/github_oauth.go b/pkg/login/social/connectors/github_oauth.go index 7506f21348d..cac4942b73b 100644 --- a/pkg/login/social/connectors/github_oauth.go +++ b/pkg/login/social/connectors/github_oauth.go @@ -76,6 +76,18 @@ func NewGitHubProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettings sso } func (s *SocialGithub) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { + info, err := CreateOAuthInfoFromKeyValues(settings.Settings) + if err != nil { + return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) + } + + err = validateInfo(info) + if err != nil { + return err + } + + // add specific validation rules for Github + return nil } diff --git a/pkg/login/social/connectors/github_oauth_test.go b/pkg/login/social/connectors/github_oauth_test.go index 336fee64569..bbc1c23f8da 100644 --- a/pkg/login/social/connectors/github_oauth_test.go +++ b/pkg/login/social/connectors/github_oauth_test.go @@ -13,6 +13,7 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/services/featuremgmt" + ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" "github.com/grafana/grafana/pkg/setting" ) @@ -341,3 +342,60 @@ func TestSocialGitHub_InitializeExtraFields(t *testing.T) { }) } } + +func TestSocialGitHub_Validate(t *testing.T) { + testCases := []struct { + name string + settings ssoModels.SSOSettings + expectError bool + }{ + { + name: "SSOSettings is valid", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + }, + }, + expectError: false, + }, + { + name: "fails if settings map contains an invalid field", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "invalid_field": []int{1, 2, 3}, + }, + }, + expectError: true, + }, + { + name: "fails if client id is empty", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "", + }, + }, + expectError: true, + }, + { + name: "fails if client id does not exist", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{}, + }, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + s := NewGitHubProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) + + err := s.Validate(context.Background(), tc.settings) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/login/social/connectors/gitlab_oauth.go b/pkg/login/social/connectors/gitlab_oauth.go index 2bce4283bb4..850083ad8e7 100644 --- a/pkg/login/social/connectors/gitlab_oauth.go +++ b/pkg/login/social/connectors/gitlab_oauth.go @@ -66,6 +66,18 @@ func NewGitLabProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettings sso } func (s *SocialGitlab) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { + info, err := CreateOAuthInfoFromKeyValues(settings.Settings) + if err != nil { + return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) + } + + err = validateInfo(info) + if err != nil { + return err + } + + // add specific validation rules for Gitlab + return nil } diff --git a/pkg/login/social/connectors/gitlab_oauth_test.go b/pkg/login/social/connectors/gitlab_oauth_test.go index 3c068cb01b8..df0c3100cbb 100644 --- a/pkg/login/social/connectors/gitlab_oauth_test.go +++ b/pkg/login/social/connectors/gitlab_oauth_test.go @@ -18,6 +18,7 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" + ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" "github.com/grafana/grafana/pkg/setting" ) @@ -459,3 +460,60 @@ func TestSocialGitlab_GetGroupsNextPage(t *testing.T) { assert.Equal(t, expectedGroups, actualGroups) assert.Equal(t, 2, calls) } + +func TestSocialGitlab_Validate(t *testing.T) { + testCases := []struct { + name string + settings ssoModels.SSOSettings + expectError bool + }{ + { + name: "SSOSettings is valid", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + }, + }, + expectError: false, + }, + { + name: "fails if settings map contains an invalid field", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "invalid_field": []int{1, 2, 3}, + }, + }, + expectError: true, + }, + { + name: "fails if client id is empty", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "", + }, + }, + expectError: true, + }, + { + name: "fails if client id does not exist", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{}, + }, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + s := NewGitLabProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) + + err := s.Validate(context.Background(), tc.settings) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/login/social/connectors/google_oauth.go b/pkg/login/social/connectors/google_oauth.go index bffc8f91b1c..440ebb5e432 100644 --- a/pkg/login/social/connectors/google_oauth.go +++ b/pkg/login/social/connectors/google_oauth.go @@ -56,6 +56,18 @@ func NewGoogleProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettings sso } func (s *SocialGoogle) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { + info, err := CreateOAuthInfoFromKeyValues(settings.Settings) + if err != nil { + return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) + } + + err = validateInfo(info) + if err != nil { + return err + } + + // add specific validation rules for Google + return nil } diff --git a/pkg/login/social/connectors/google_oauth_test.go b/pkg/login/social/connectors/google_oauth_test.go index dffd3d74bc4..c386311cb6c 100644 --- a/pkg/login/social/connectors/google_oauth_test.go +++ b/pkg/login/social/connectors/google_oauth_test.go @@ -17,6 +17,7 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/models/roletype" "github.com/grafana/grafana/pkg/services/featuremgmt" + ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" "github.com/grafana/grafana/pkg/setting" ) @@ -664,3 +665,60 @@ func TestSocialGoogle_UserInfo(t *testing.T) { }) } } + +func TestSocialGoogle_Validate(t *testing.T) { + testCases := []struct { + name string + settings ssoModels.SSOSettings + expectError bool + }{ + { + name: "SSOSettings is valid", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + }, + }, + expectError: false, + }, + { + name: "fails if settings map contains an invalid field", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "invalid_field": []int{1, 2, 3}, + }, + }, + expectError: true, + }, + { + name: "fails if client id is empty", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "", + }, + }, + expectError: true, + }, + { + name: "fails if client id does not exist", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{}, + }, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + s := NewGoogleProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) + + err := s.Validate(context.Background(), tc.settings) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/login/social/connectors/grafana_com_oauth.go b/pkg/login/social/connectors/grafana_com_oauth.go index d3bda3607ef..a32ad8be3cc 100644 --- a/pkg/login/social/connectors/grafana_com_oauth.go +++ b/pkg/login/social/connectors/grafana_com_oauth.go @@ -54,6 +54,18 @@ func NewGrafanaComProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettings } func (s *SocialGrafanaCom) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { + info, err := CreateOAuthInfoFromKeyValues(settings.Settings) + if err != nil { + return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) + } + + err = validateInfo(info) + if err != nil { + return err + } + + // add specific validation rules for GrafanaCom + return nil } diff --git a/pkg/login/social/connectors/grafana_com_oauth_test.go b/pkg/login/social/connectors/grafana_com_oauth_test.go index 63c0c668b89..9f827bcfc7c 100644 --- a/pkg/login/social/connectors/grafana_com_oauth_test.go +++ b/pkg/login/social/connectors/grafana_com_oauth_test.go @@ -10,6 +10,7 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/services/featuremgmt" + ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" "github.com/grafana/grafana/pkg/setting" ) @@ -132,3 +133,60 @@ func TestSocialGrafanaCom_InitializeExtraFields(t *testing.T) { }) } } + +func TestSocialGrafanaCom_Validate(t *testing.T) { + testCases := []struct { + name string + settings ssoModels.SSOSettings + expectError bool + }{ + { + name: "SSOSettings is valid", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + }, + }, + expectError: false, + }, + { + name: "fails if settings map contains an invalid field", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "invalid_field": []int{1, 2, 3}, + }, + }, + expectError: true, + }, + { + name: "fails if client id is empty", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "", + }, + }, + expectError: true, + }, + { + name: "fails if client id does not exist", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{}, + }, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + s := NewGrafanaComProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) + + err := s.Validate(context.Background(), tc.settings) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/login/social/connectors/okta_oauth.go b/pkg/login/social/connectors/okta_oauth.go index 89f043c54c5..4de9bcca9ec 100644 --- a/pkg/login/social/connectors/okta_oauth.go +++ b/pkg/login/social/connectors/okta_oauth.go @@ -62,6 +62,18 @@ func NewOktaProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettings ssose } func (s *SocialOkta) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { + info, err := CreateOAuthInfoFromKeyValues(settings.Settings) + if err != nil { + return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) + } + + err = validateInfo(info) + if err != nil { + return err + } + + // add specific validation rules for Okta + return nil } diff --git a/pkg/login/social/connectors/okta_oauth_test.go b/pkg/login/social/connectors/okta_oauth_test.go index 27019a03280..2ebcdf50af5 100644 --- a/pkg/login/social/connectors/okta_oauth_test.go +++ b/pkg/login/social/connectors/okta_oauth_test.go @@ -15,6 +15,7 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/models/roletype" "github.com/grafana/grafana/pkg/services/featuremgmt" + ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" "github.com/grafana/grafana/pkg/setting" ) @@ -132,3 +133,60 @@ func TestSocialOkta_UserInfo(t *testing.T) { }) } } + +func TestSocialOkta_Validate(t *testing.T) { + testCases := []struct { + name string + settings ssoModels.SSOSettings + expectError bool + }{ + { + name: "SSOSettings is valid", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + }, + }, + expectError: false, + }, + { + name: "fails if settings map contains an invalid field", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "invalid_field": []int{1, 2, 3}, + }, + }, + expectError: true, + }, + { + name: "fails if client id is empty", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "", + }, + }, + expectError: true, + }, + { + name: "fails if client id does not exist", + settings: ssoModels.SSOSettings{ + Settings: map[string]any{}, + }, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + s := NewOktaProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) + + err := s.Validate(context.Background(), tc.settings) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/login/social/connectors/social_base.go b/pkg/login/social/connectors/social_base.go index 5ee11e3be20..f8ecd5216b3 100644 --- a/pkg/login/social/connectors/social_base.go +++ b/pkg/login/social/connectors/social_base.go @@ -18,6 +18,7 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/services/ssosettings" ) type SocialBase struct { @@ -209,3 +210,11 @@ func getRoleFromSearch(role string) (org.RoleType, bool) { return org.RoleType(cases.Title(language.Und).String(role)), false } + +func validateInfo(info *social.OAuthInfo) error { + if info.ClientId == "" { + return ssosettings.ErrEmptyClientId.Errorf("clientId is empty") + } + + return nil +} diff --git a/pkg/services/ssosettings/errors.go b/pkg/services/ssosettings/errors.go index 3bc22582ec0..4f2b0981b9d 100644 --- a/pkg/services/ssosettings/errors.go +++ b/pkg/services/ssosettings/errors.go @@ -1,7 +1,14 @@ package ssosettings -import "errors" +import ( + "errors" + + "github.com/grafana/grafana/pkg/util/errutil" +) var ( ErrNotFound = errors.New("not found") + + ErrInvalidSettings = errutil.ValidationFailed("sso.settings", errutil.WithPublicMessage("settings field is invalid")) + ErrEmptyClientId = errutil.ValidationFailed("sso.emptyClientId", errutil.WithPublicMessage("settings.clientId cannot be empty")) )