From 8246d97587e9caa9e7513f2ed917ae33af2f0d3b Mon Sep 17 00:00:00 2001 From: Misi Date: Tue, 23 Jan 2024 15:48:06 +0100 Subject: [PATCH] Auth: Introduce `configurable_providers` config option for SSO settings (#80911) * Add SSOSettingsConfigurableProviders config option * Add check to Delete and ListWithRedactedSecrets * Add check to GET, small improvements --- conf/defaults.ini | 3 ++ pkg/services/ssosettings/api/api.go | 12 +---- pkg/services/ssosettings/api/api_test.go | 10 +++++ pkg/services/ssosettings/errors.go | 7 +-- .../ssosettings/ssosettingsimpl/service.go | 37 +++++++++------ .../ssosettingsimpl/service_test.go | 45 ++++++++++--------- pkg/setting/setting.go | 8 +++- public/app/features/auth-config/utils/url.ts | 2 +- 8 files changed, 75 insertions(+), 49 deletions(-) diff --git a/conf/defaults.ini b/conf/defaults.ini index 0b1f7b359b2..e01a8df0c63 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -584,6 +584,9 @@ id_response_header_namespaces = user api-key service-account # set to 0 to disable this feature reload_interval = 1m +# List of providers that can be configured through the SSO Settings API and UI. +configurable_providers = github gitlab google generic_oauth azuread okta + #################################### Anonymous Auth ###################### [auth.anonymous] # enable anonymous access diff --git a/pkg/services/ssosettings/api/api.go b/pkg/services/ssosettings/api/api.go index d2dc4ba4ac1..66357222650 100644 --- a/pkg/services/ssosettings/api/api.go +++ b/pkg/services/ssosettings/api/api.go @@ -3,7 +3,6 @@ package api import ( "context" "encoding/json" - "errors" "fmt" "hash/fnv" "net/http" @@ -140,12 +139,8 @@ func (api *Api) getProviderSettings(c *contextmodel.ReqContext) response.Respons } provider, err := api.SSOSettingsService.GetForProviderWithRedactedSecrets(c.Req.Context(), key) - if err != nil { - if errors.Is(err, ssosettings.ErrNotFound) { - return response.Error(http.StatusNotFound, "The provider was not found", err) - } - return response.Error(http.StatusInternalServerError, "Failed to get provider settings", err) + return response.ErrOrFallback(http.StatusInternalServerError, "Failed to get provider settings", err) } etag, err := generateFNVETag(provider) @@ -214,10 +209,7 @@ func (api *Api) removeProviderSettings(c *contextmodel.ReqContext) response.Resp err := api.SSOSettingsService.Delete(c.Req.Context(), key) if err != nil { - if errors.Is(err, ssosettings.ErrNotFound) { - return response.Error(http.StatusNotFound, "The provider was not found", err) - } - return response.Error(http.StatusInternalServerError, "Failed to delete provider settings", err) + return response.ErrOrFallback(http.StatusInternalServerError, "Failed to delete provider settings", err) } return response.Empty(http.StatusNoContent) diff --git a/pkg/services/ssosettings/api/api_test.go b/pkg/services/ssosettings/api/api_test.go index 1da658af811..3421c4e5dcb 100644 --- a/pkg/services/ssosettings/api/api_test.go +++ b/pkg/services/ssosettings/api/api_test.go @@ -336,6 +336,16 @@ func TestSSOSettingsAPI_GetForProvider(t *testing.T) { expectedServiceCall: true, expectedStatusCode: http.StatusInternalServerError, }, + { + desc: "fails with not found error when the provider is not configurable", + key: "grafana_com", + action: "settings:read", + scope: "settings:*", + expectedResult: nil, + expectedError: ssosettings.ErrNotConfigurable, + expectedServiceCall: true, + expectedStatusCode: http.StatusNotFound, + }, } for _, tt := range tests { diff --git a/pkg/services/ssosettings/errors.go b/pkg/services/ssosettings/errors.go index 83563ff28de..6c548624625 100644 --- a/pkg/services/ssosettings/errors.go +++ b/pkg/services/ssosettings/errors.go @@ -1,13 +1,14 @@ package ssosettings import ( - "errors" - "github.com/grafana/grafana/pkg/util/errutil" ) var ( - ErrNotFound = errors.New("not found") + errNotFoundBase = errutil.NotFound("sso.notFound", errutil.WithPublicMessage("The provider was not found.")) + ErrNotFound = errNotFoundBase.Errorf("not found") + + ErrNotConfigurable = errNotFoundBase.Errorf("not configurable") ErrInvalidProvider = errutil.ValidationFailed("sso.invalidProvider", errutil.WithPublicMessage("Provider is invalid")) ErrInvalidSettings = errutil.ValidationFailed("sso.settings", errutil.WithPublicMessage("Settings field is invalid")) diff --git a/pkg/services/ssosettings/ssosettingsimpl/service.go b/pkg/services/ssosettings/ssosettingsimpl/service.go index 6ef210ff71b..7c25ad5743b 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service.go @@ -88,6 +88,10 @@ func (s *SSOSettingsService) GetForProvider(ctx context.Context, provider string } func (s *SSOSettingsService) GetForProviderWithRedactedSecrets(ctx context.Context, provider string) (*models.SSOSettings, error) { + if !s.isProviderConfigurable(provider) { + return nil, ssosettings.ErrNotConfigurable + } + storeSettings, err := s.GetForProvider(ctx, provider) if err != nil { return nil, err @@ -136,7 +140,14 @@ func (s *SSOSettingsService) ListWithRedactedSecrets(ctx context.Context) ([]*mo return nil, err } - for _, storeSetting := range storeSettings { + configurableSettings := make([]*models.SSOSettings, 0, len(s.cfg.SSOSettingsConfigurableProviders)) + for _, provider := range storeSettings { + if s.isProviderConfigurable(provider.Provider) { + configurableSettings = append(configurableSettings, provider) + } + } + + for _, storeSetting := range configurableSettings { for k, v := range storeSetting.Settings { if strVal, ok := v.(string); ok { storeSetting.Settings[k] = setting.RedactedValue(k, strVal) @@ -144,12 +155,12 @@ func (s *SSOSettingsService) ListWithRedactedSecrets(ctx context.Context) ([]*mo } } - return storeSettings, nil + return configurableSettings, nil } func (s *SSOSettingsService) Upsert(ctx context.Context, settings *models.SSOSettings) error { - if !isProviderConfigurable(settings.Provider) { - return ssosettings.ErrInvalidProvider.Errorf("provider %s is not configurable", settings.Provider) + if !s.isProviderConfigurable(settings.Provider) { + return ssosettings.ErrNotConfigurable } social, ok := s.reloadables[settings.Provider] @@ -195,6 +206,9 @@ func (s *SSOSettingsService) Patch(ctx context.Context, provider string, data ma } func (s *SSOSettingsService) Delete(ctx context.Context, provider string) error { + if !s.isProviderConfigurable(provider) { + return ssosettings.ErrNotConfigurable + } return s.store.Delete(ctx, provider) } @@ -365,6 +379,11 @@ func (s *SSOSettingsService) decryptSecrets(ctx context.Context, settings map[st return settings, nil } +func (s *SSOSettingsService) isProviderConfigurable(provider string) bool { + _, ok := s.cfg.SSOSettingsConfigurableProviders[provider] + return ok +} + // removeSecrets removes all the secrets from the map and replaces them with a redacted password // and returns a new map func removeSecrets(settings map[string]any) map[string]any { @@ -434,16 +453,6 @@ func isSecret(fieldName string) bool { return false } -func isProviderConfigurable(provider string) bool { - for _, configurable := range ssosettings.ConfigurableOAuthProviders { - if provider == configurable { - return true - } - } - - return false -} - func isNewSecretValue(value string) bool { return value != setting.RedactedPassword } diff --git a/pkg/services/ssosettings/ssosettingsimpl/service_test.go b/pkg/services/ssosettings/ssosettingsimpl/service_test.go index a0c1aca5778..cdc58760921 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service_test.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service_test.go @@ -588,16 +588,6 @@ func TestSSOSettingsService_ListWithRedactedSecrets(t *testing.T) { }, Source: models.System, }, - { - Provider: "grafana_com", - Settings: map[string]any{ - "enabled": true, - "secret": "*********", - "client_secret": "*********", - "client_id": "client_id", - }, - Source: models.System, - }, }, wantErr: false, }, @@ -718,16 +708,6 @@ func TestSSOSettingsService_ListWithRedactedSecrets(t *testing.T) { }, Source: models.System, }, - { - Provider: "grafana_com", - Settings: map[string]any{ - "enabled": false, - "secret": "*********", - "client_secret": "*********", - "client_id": "client_id", - }, - Source: models.System, - }, }, wantErr: false, }, @@ -1045,6 +1025,17 @@ func TestSSOSettingsService_Delete(t *testing.T) { require.ErrorIs(t, err, ssosettings.ErrNotFound) }) + t.Run("should not delete the SSO settings if the provider is not configurable", func(t *testing.T) { + env := setupTestEnv(t) + env.cfg.SSOSettingsConfigurableProviders = map[string]bool{social.AzureADProviderName: true} + + provider := social.GrafanaComProviderName + env.store.ExpectedError = nil + + err := env.service.Delete(context.Background(), provider) + require.ErrorIs(t, err, ssosettings.ErrNotConfigurable) + }) + t.Run("store fails to delete the SSO settings for the specified provider", func(t *testing.T) { env := setupTestEnv(t) @@ -1214,8 +1205,20 @@ func setupTestEnv(t *testing.T) testEnv { fallbackStrategy.ExpectedIsMatch = true + cfg := &setting.Cfg{ + SSOSettingsConfigurableProviders: map[string]bool{ + "github": true, + "okta": true, + "azuread": true, + "google": true, + "generic_oauth": true, + "gitlab": true, + }, + } + svc := &SSOSettingsService{ logger: log.NewNopLogger(), + cfg: cfg, store: store, ac: accessControl, fbStrategies: []ssosettings.FallbackStrategy{fallbackStrategy}, @@ -1224,6 +1227,7 @@ func setupTestEnv(t *testing.T) testEnv { } return testEnv{ + cfg: cfg, service: svc, store: store, ac: accessControl, @@ -1234,6 +1238,7 @@ func setupTestEnv(t *testing.T) testEnv { } type testEnv struct { + cfg *setting.Cfg service *SSOSettingsService store *ssosettingstests.FakeStore ac accesscontrol.AccessControl diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index b487ac37a47..10d3b54289a 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -287,7 +287,8 @@ type Cfg struct { ExtendedJWTExpectAudience string // SSO Settings Auth - SSOSettingsReloadInterval time.Duration + SSOSettingsReloadInterval time.Duration + SSOSettingsConfigurableProviders map[string]bool // Dataproxy SendUserHeader bool @@ -1628,7 +1629,12 @@ func readAuthSettings(iniFile *ini.File, cfg *Cfg) (err error) { // SSO Settings ssoSettings := iniFile.Section("sso_settings") cfg.SSOSettingsReloadInterval = ssoSettings.Key("reload_interval").MustDuration(1 * time.Minute) + providers := ssoSettings.Key("configurable_providers").String() + cfg.SSOSettingsConfigurableProviders = make(map[string]bool) + for _, provider := range util.SplitString(providers) { + cfg.SSOSettingsConfigurableProviders[provider] = true + } return nil } diff --git a/public/app/features/auth-config/utils/url.ts b/public/app/features/auth-config/utils/url.ts index bb5c435d544..089f72b7955 100644 --- a/public/app/features/auth-config/utils/url.ts +++ b/public/app/features/auth-config/utils/url.ts @@ -2,5 +2,5 @@ import { BASE_PATH } from '../constants'; import { AuthProviderInfo } from '../types'; export function getProviderUrl(provider: AuthProviderInfo) { - return BASE_PATH + (provider.configPath || `advanced/${provider.id}`); + return BASE_PATH + (provider.configPath || provider.id); }