From 3b7e7483c81c597e991ba7812d6ae67a64e9a67a Mon Sep 17 00:00:00 2001 From: Misi Date: Wed, 28 Feb 2024 13:45:59 +0100 Subject: [PATCH] Auth: Align loading the legacy auth.grafananet section to the current behaviour in OAuthStrategy (#83479) * Align oauth_strategy to the current behaviour * lint * Address feedback --- pkg/login/social/socialimpl/service_test.go | 95 +++++++++++++++++++ .../ssosettings/strategies/oauth_strategy.go | 9 +- .../strategies/oauth_strategy_test.go | 93 +++++++++++++++++- 3 files changed, 195 insertions(+), 2 deletions(-) diff --git a/pkg/login/social/socialimpl/service_test.go b/pkg/login/social/socialimpl/service_test.go index a0f80172de4..04fa8b403f0 100644 --- a/pkg/login/social/socialimpl/service_test.go +++ b/pkg/login/social/socialimpl/service_test.go @@ -91,6 +91,101 @@ func TestSocialService_ProvideService(t *testing.T) { } } +func TestSocialService_ProvideService_GrafanaComGrafanaNet(t *testing.T) { + testCases := []struct { + name string + rawIniContent string + expectedGrafanaComOAuthInfo *social.OAuthInfo + }{ + { + name: "should setup the connector using auth.grafana_com section if it is enabled", + rawIniContent: ` + [auth.grafana_com] + enabled = true + client_id = grafanaComClientId + + [auth.grafananet] + enabled = false + client_id = grafanaNetClientId`, + expectedGrafanaComOAuthInfo: &social.OAuthInfo{ + AuthStyle: "inheader", + AuthUrl: "/oauth2/authorize", + TokenUrl: "/api/oauth2/token", + Enabled: true, + ClientId: "grafanaComClientId", + }, + }, + { + name: "should setup the connector using auth.grafananet section if it is enabled", + rawIniContent: ` + [auth.grafana_com] + enabled = false + client_id = grafanaComClientId + + [auth.grafananet] + enabled = true + client_id = grafanaNetClientId`, + expectedGrafanaComOAuthInfo: &social.OAuthInfo{ + AuthStyle: "inheader", + AuthUrl: "/oauth2/authorize", + TokenUrl: "/api/oauth2/token", + Enabled: true, + ClientId: "grafanaNetClientId", + }, + }, + { + name: "should setup the connector using auth.grafana_com section if both are enabled", + rawIniContent: ` + [auth.grafana_com] + enabled = true + client_id = grafanaComClientId + + [auth.grafananet] + enabled = true + client_id = grafanaNetClientId`, + expectedGrafanaComOAuthInfo: &social.OAuthInfo{ + AuthStyle: "inheader", + AuthUrl: "/oauth2/authorize", + TokenUrl: "/api/oauth2/token", + Enabled: true, + ClientId: "grafanaComClientId", + }, + }, + { + name: "should not setup the connector when both are disabled", + rawIniContent: ` + [auth.grafana_com] + enabled = false + client_id = grafanaComClientId + + [auth.grafananet] + enabled = false + client_id = grafanaNetClientId`, + expectedGrafanaComOAuthInfo: nil, + }, + } + + cfg := setting.NewCfg() + secrets := secretsfake.NewMockService(t) + accessControl := acimpl.ProvideAccessControl(cfg) + sqlStore := db.InitTestDB(t) + + ssoSettingsSvc := ssosettingsimpl.ProvideService(cfg, sqlStore, accessControl, routing.NewRouteRegister(), featuremgmt.WithFeatures(), secrets, &usagestats.UsageStatsMock{}, nil) + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + iniFile, err := ini.Load([]byte(tc.rawIniContent)) + require.NoError(t, err) + + cfg := setting.NewCfg() + cfg.Raw = iniFile + + socialService := ProvideService(cfg, featuremgmt.WithFeatures(), &usagestats.UsageStatsMock{}, supportbundlestest.NewFakeBundleService(), remotecache.NewFakeStore(t), ssoSettingsSvc) + require.EqualValues(t, tc.expectedGrafanaComOAuthInfo, socialService.GetOAuthInfoProvider("grafana_com")) + }) + } +} + func TestMapping_IniSectionOAuthInfo(t *testing.T) { iniContent := ` [test] diff --git a/pkg/services/ssosettings/strategies/oauth_strategy.go b/pkg/services/ssosettings/strategies/oauth_strategy.go index 215d0a5cf02..4dbbfddfbe0 100644 --- a/pkg/services/ssosettings/strategies/oauth_strategy.go +++ b/pkg/services/ssosettings/strategies/oauth_strategy.go @@ -51,13 +51,20 @@ func (s *OAuthStrategy) loadAllSettings() { allProviders := append(ssosettings.AllOAuthProviders, social.GrafanaNetProviderName) for _, provider := range allProviders { settings := s.loadSettingsForProvider(provider) - if provider == social.GrafanaNetProviderName { + // This is required to support the legacy settings for the provider (auth.grafananet section) + // It will use the settings (and overwrite the current grafana_com settings) from auth.grafananet if + // the auth.grafananet section is enabled and the auth.grafana_com section is disabled. + if provider == social.GrafanaNetProviderName && s.shouldUseGrafanaNetSettings() && settings["enabled"] == true { provider = social.GrafanaComProviderName } s.settingsByProvider[provider] = settings } } +func (s *OAuthStrategy) shouldUseGrafanaNetSettings() bool { + return s.settingsByProvider[social.GrafanaComProviderName]["enabled"] == false +} + func (s *OAuthStrategy) loadSettingsForProvider(provider string) map[string]any { section := s.cfg.Raw.Section("auth." + provider) diff --git a/pkg/services/ssosettings/strategies/oauth_strategy_test.go b/pkg/services/ssosettings/strategies/oauth_strategy_test.go index c5e4cc28bec..b41e14e1b3f 100644 --- a/pkg/services/ssosettings/strategies/oauth_strategy_test.go +++ b/pkg/services/ssosettings/strategies/oauth_strategy_test.go @@ -127,6 +127,7 @@ func TestGetProviderConfig_ExtraFields(t *testing.T) { allowed_organizations = org1, org2 [auth.grafana_com] + enabled = true allowed_organizations = org1, org2 ` @@ -166,10 +167,100 @@ func TestGetProviderConfig_ExtraFields(t *testing.T) { }) t.Run("grafana_com", func(t *testing.T) { - t.Skip("Skipping to revert an issue.") result, err := strategy.GetProviderConfig(context.Background(), "grafana_com") require.NoError(t, err) require.Equal(t, "org1, org2", result["allowed_organizations"]) }) } + +// TestGetProviderConfig_GrafanaComGrafanaNet tests that the connector is setup using the correct section and it supports +// the legacy settings for the provider (auth.grafananet section). The test cases are based on the current behavior of the +// SocialService's ProvideService method (TestSocialService_ProvideService_GrafanaComGrafanaNet). +func TestGetProviderConfig_GrafanaComGrafanaNet(t *testing.T) { + testCases := []struct { + name string + rawIniContent string + expectedGrafanaComSettings map[string]any + }{ + { + name: "should setup the connector using auth.grafana_com section if it is enabled", + rawIniContent: ` + [auth.grafana_com] + enabled = true + client_id = grafanaComClientId + + [auth.grafananet] + enabled = false + client_id = grafanaNetClientId`, + expectedGrafanaComSettings: map[string]any{ + "enabled": true, + "client_id": "grafanaComClientId", + }, + }, + { + name: "should setup the connector using auth.grafananet section if it is enabled", + rawIniContent: ` + [auth.grafana_com] + enabled = false + client_id = grafanaComClientId + + [auth.grafananet] + enabled = true + client_id = grafanaNetClientId`, + expectedGrafanaComSettings: map[string]any{ + "enabled": true, + "client_id": "grafanaNetClientId", + }, + }, + { + name: "should setup the connector using auth.grafana_com section if both are enabled", + rawIniContent: ` + [auth.grafana_com] + enabled = true + client_id = grafanaComClientId + + [auth.grafananet] + enabled = true + client_id = grafanaNetClientId`, + expectedGrafanaComSettings: map[string]any{ + "enabled": true, + "client_id": "grafanaComClientId", + }, + }, + { + name: "should not setup the connector when both are disabled", + rawIniContent: ` + [auth.grafana_com] + enabled = false + client_id = grafanaComClientId + + [auth.grafananet] + enabled = false + client_id = grafanaNetClientId`, + expectedGrafanaComSettings: map[string]any{ + "enabled": false, + "client_id": "grafanaComClientId", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + iniFile, err := ini.Load([]byte(tc.rawIniContent)) + require.NoError(t, err) + + cfg := setting.NewCfg() + cfg.Raw = iniFile + + strategy := NewOAuthStrategy(cfg) + + actualConfig, err := strategy.GetProviderConfig(context.Background(), "grafana_com") + require.NoError(t, err) + + for key, value := range tc.expectedGrafanaComSettings { + require.Equal(t, value, actualConfig[key], "Difference in key: %s. Expected: %v, got: %v", key, value, actualConfig[key]) + } + }) + } +}