Refactor SSOSettings to use types (#78675)

* refactor SSOSettings to use types

* test struct

* refactor SSOSettings struct to use types

* fix database tests

* fix populateSSOSettings() to accept an SSOSettings param

* fix all tests from the database layer

* handle errors for converting to/from SSOSettings

* add json tag on OAuthInfo fields

* use continue instead of if/else

* add the source field to SSOSettingsDTO conversion

* remove omitempty from json tags in OAuthInfo struct

pkg/services/ssosettings/ssosettingsimpl/service.go

@@ -7,6 +7,7 @@ import (
 	"github.com/grafana/grafana/pkg/api/routing"
 	"github.com/grafana/grafana/pkg/infra/db"
 	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/login/social"
 	ac "github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/services/auth/identity"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
@@ -55,29 +56,29 @@ func ProvideService(cfg *setting.Cfg, sqlStore db.DB, ac ac.AccessControl,
 
 var _ ssosettings.Service = (*SSOSettingsService)(nil)
 
-func (s *SSOSettingsService) GetForProvider(ctx context.Context, provider string) (*models.SSOSetting, error) {
-	dto, err := s.store.Get(ctx, provider)
+func (s *SSOSettingsService) GetForProvider(ctx context.Context, provider string) (*models.SSOSettings, error) {
+	storeSettings, err := s.store.Get(ctx, provider)
 
 	if errors.Is(err, ssosettings.ErrNotFound) {
-		setting, err := s.loadSettingsUsingFallbackStrategy(ctx, provider)
+		settings, err := s.loadSettingsUsingFallbackStrategy(ctx, provider)
 		if err != nil {
 			return nil, err
 		}
 
-		return setting, nil
+		return settings, nil
 	}
 
 	if err != nil {
 		return nil, err
 	}
 
-	dto.Source = models.DB
+	storeSettings.Source = models.DB
 
-	return dto, nil
+	return storeSettings, nil
 }
 
-func (s *SSOSettingsService) List(ctx context.Context, requester identity.Requester) ([]*models.SSOSetting, error) {
-	result := make([]*models.SSOSetting, 0, len(ssosettings.AllOAuthProviders))
+func (s *SSOSettingsService) List(ctx context.Context, requester identity.Requester) ([]*models.SSOSettings, error) {
+	result := make([]*models.SSOSettings, 0, len(ssosettings.AllOAuthProviders))
 	storedSettings, err := s.store.List(ctx)
 
 	if err != nil {
@@ -98,12 +99,12 @@ func (s *SSOSettingsService) List(ctx context.Context, requester identity.Reques
 		settings := getSettingsByProvider(provider, storedSettings)
 		if len(settings) == 0 {
 			// If there is no data in the DB then we need to load the settings using the fallback strategy
-			setting, err := s.loadSettingsUsingFallbackStrategy(ctx, provider)
+			fallbackSettings, err := s.loadSettingsUsingFallbackStrategy(ctx, provider)
 			if err != nil {
 				return nil, err
 			}
 
-			settings = append(settings, setting)
+			settings = append(settings, fallbackSettings)
 		}
 		result = append(result, settings...)
 	}
@@ -111,9 +112,9 @@ func (s *SSOSettingsService) List(ctx context.Context, requester identity.Reques
 	return result, nil
 }
 
-func (s *SSOSettingsService) Upsert(ctx context.Context, provider string, data map[string]interface{}) error {
+func (s *SSOSettingsService) Upsert(ctx context.Context, settings models.SSOSettings) error {
 	// TODO: validation (configurable provider? Contains the required fields? etc)
-	err := s.store.Upsert(ctx, provider, data)
+	err := s.store.Upsert(ctx, settings)
 	if err != nil {
 		return err
 	}
@@ -140,7 +141,7 @@ func (s *SSOSettingsService) RegisterFallbackStrategy(providerRegex string, stra
 	s.fbStrategies = append(s.fbStrategies, strategy)
 }
 
-func (s *SSOSettingsService) loadSettingsUsingFallbackStrategy(ctx context.Context, provider string) (*models.SSOSetting, error) {
+func (s *SSOSettingsService) loadSettingsUsingFallbackStrategy(ctx context.Context, provider string) (*models.SSOSettings, error) {
 	loadStrategy, ok := s.getFallBackstrategyFor(provider)
 	if !ok {
 		return nil, errors.New("no fallback strategy found for provider: " + provider)
@@ -151,18 +152,23 @@ func (s *SSOSettingsService) loadSettingsUsingFallbackStrategy(ctx context.Conte
 		return nil, err
 	}
 
-	return &models.SSOSetting{
-		Provider: provider,
-		Source:   models.System,
-		Settings: settingsFromSystem,
+	oAuthInfo, err := social.CreateOAuthInfoFromKeyValues(settingsFromSystem)
+	if err != nil {
+		return nil, err
+	}
+
+	return &models.SSOSettings{
+		Provider:      provider,
+		Source:        models.System,
+		OAuthSettings: oAuthInfo,
 	}, nil
 }
 
-func getSettingsByProvider(provider string, settings []*models.SSOSetting) []*models.SSOSetting {
-	result := make([]*models.SSOSetting, 0)
-	for _, setting := range settings {
-		if setting.Provider == provider {
-			result = append(result, setting)
+func getSettingsByProvider(provider string, settings []*models.SSOSettings) []*models.SSOSettings {
+	result := make([]*models.SSOSettings, 0)
+	for _, item := range settings {
+		if item.Provider == provider {
+			result = append(result, item)
 		}
 	}
 	return result

pkg/services/ssosettings/ssosettingsimpl/service_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/login/social"
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/services/accesscontrol/acimpl"
 	"github.com/grafana/grafana/pkg/services/auth/identity"
@@ -22,25 +23,21 @@ func TestSSOSettingsService_GetForProvider(t *testing.T) {
 	testCases := []struct {
 		name    string
 		setup   func(env testEnv)
-		want    *models.SSOSetting
+		want    *models.SSOSettings
 		wantErr bool
 	}{
 		{
 			name: "should return successfully",
 			setup: func(env testEnv) {
-				env.store.ExpectedSSOSetting = &models.SSOSetting{
-					Provider: "github",
-					Settings: map[string]interface{}{
-						"enabled": true,
-					},
-					Source: models.DB,
+				env.store.ExpectedSSOSetting = &models.SSOSettings{
+					Provider:      "github",
+					OAuthSettings: &social.OAuthInfo{Enabled: true},
+					Source:        models.DB,
 				}
 			},
-			want: &models.SSOSetting{
-				Provider: "github",
-				Settings: map[string]interface{}{
-					"enabled": true,
-				},
+			want: &models.SSOSettings{
+				Provider:      "github",
+				OAuthSettings: &social.OAuthInfo{Enabled: true},
 			},
 			wantErr: false,
 		},
@@ -59,12 +56,10 @@ func TestSSOSettingsService_GetForProvider(t *testing.T) {
 					"enabled": true,
 				}
 			},
-			want: &models.SSOSetting{
-				Provider: "github",
-				Settings: map[string]interface{}{
-					"enabled": true,
-				},
-				Source: models.System,
+			want: &models.SSOSettings{
+				Provider:      "github",
+				OAuthSettings: &social.OAuthInfo{Enabled: true},
+				Source:        models.System,
 			},
 			wantErr: false,
 		},
@@ -136,26 +131,22 @@ func TestSSOSettingsService_List(t *testing.T) {
 		name     string
 		setup    func(env testEnv)
 		identity identity.Requester
-		want     []*models.SSOSetting
+		want     []*models.SSOSettings
 		wantErr  bool
 	}{
 		{
 			name: "should return successfully",
 			setup: func(env testEnv) {
-				env.store.ExpectedSSOSettings = []*models.SSOSetting{
+				env.store.ExpectedSSOSettings = []*models.SSOSettings{
 					{
-						Provider: "github",
-						Settings: map[string]interface{}{
-							"enabled": true,
-						},
-						Source: models.DB,
+						Provider:      "github",
+						OAuthSettings: &social.OAuthInfo{Enabled: true},
+						Source:        models.DB,
 					},
 					{
-						Provider: "okta",
-						Settings: map[string]interface{}{
-							"enabled": false,
-						},
-						Source: models.DB,
+						Provider:      "okta",
+						OAuthSettings: &social.OAuthInfo{Enabled: false},
+						Source:        models.DB,
 					},
 				}
 				env.fallbackStrategy.ExpectedIsMatch = true
@@ -164,55 +155,41 @@ func TestSSOSettingsService_List(t *testing.T) {
 				}
 			},
 			identity: defaultIdentity,
-			want: []*models.SSOSetting{
+			want: []*models.SSOSettings{
 				{
-					Provider: "github",
-					Settings: map[string]interface{}{
-						"enabled": true,
-					},
-					Source: models.DB,
+					Provider:      "github",
+					OAuthSettings: &social.OAuthInfo{Enabled: true},
+					Source:        models.DB,
 				},
 				{
-					Provider: "okta",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.DB,
+					Provider:      "okta",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.DB,
 				},
 				{
-					Provider: "gitlab",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "gitlab",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 				{
-					Provider: "generic_oauth",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "generic_oauth",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 				{
-					Provider: "google",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "google",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 				{
-					Provider: "azuread",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "azuread",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 				{
-					Provider: "grafana_com",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "grafana_com",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 			},
 			wantErr: false,
@@ -220,20 +197,16 @@ func TestSSOSettingsService_List(t *testing.T) {
 		{
 			name: "should return the settings that the user has access to",
 			setup: func(env testEnv) {
-				env.store.ExpectedSSOSettings = []*models.SSOSetting{
+				env.store.ExpectedSSOSettings = []*models.SSOSettings{
 					{
-						Provider: "github",
-						Settings: map[string]interface{}{
-							"enabled": true,
-						},
-						Source: models.DB,
+						Provider:      "github",
+						OAuthSettings: &social.OAuthInfo{Enabled: true},
+						Source:        models.DB,
 					},
 					{
-						Provider: "okta",
-						Settings: map[string]interface{}{
-							"enabled": false,
-						},
-						Source: models.DB,
+						Provider:      "okta",
+						OAuthSettings: &social.OAuthInfo{Enabled: true},
+						Source:        models.DB,
 					},
 				}
 				env.fallbackStrategy.ExpectedIsMatch = true
@@ -242,20 +215,16 @@ func TestSSOSettingsService_List(t *testing.T) {
 				}
 			},
 			identity: scopedIdentity,
-			want: []*models.SSOSetting{
+			want: []*models.SSOSettings{
 				{
-					Provider: "github",
-					Settings: map[string]interface{}{
-						"enabled": true,
-					},
-					Source: models.DB,
+					Provider:      "github",
+					OAuthSettings: &social.OAuthInfo{Enabled: true},
+					Source:        models.DB,
 				},
 				{
-					Provider: "azuread",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "azuread",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 			},
 			wantErr: false,
@@ -270,62 +239,48 @@ func TestSSOSettingsService_List(t *testing.T) {
 		{
 			name: "should use the fallback strategy if store returns empty list",
 			setup: func(env testEnv) {
-				env.store.ExpectedSSOSettings = []*models.SSOSetting{}
+				env.store.ExpectedSSOSettings = []*models.SSOSettings{}
 				env.fallbackStrategy.ExpectedIsMatch = true
 				env.fallbackStrategy.ExpectedConfig = map[string]interface{}{
 					"enabled": false,
 				}
 			},
 			identity: defaultIdentity,
-			want: []*models.SSOSetting{
+			want: []*models.SSOSettings{
 				{
-					Provider: "github",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "github",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 				{
-					Provider: "okta",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "okta",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 				{
-					Provider: "gitlab",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "gitlab",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 				{
-					Provider: "generic_oauth",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "generic_oauth",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 				{
-					Provider: "google",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "google",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 				{
-					Provider: "azuread",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "azuread",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 				{
-					Provider: "grafana_com",
-					Settings: map[string]interface{}{
-						"enabled": false,
-					},
-					Source: models.System,
+					Provider:      "grafana_com",
+					OAuthSettings: &social.OAuthInfo{Enabled: false},
+					Source:        models.System,
 				},
 			},
 			wantErr: false,
@@ -333,7 +288,7 @@ func TestSSOSettingsService_List(t *testing.T) {
 		{
 			name: "should return error if any of the fallback strategies was not found",
 			setup: func(env testEnv) {
-				env.store.ExpectedSSOSettings = []*models.SSOSetting{}
+				env.store.ExpectedSSOSettings = []*models.SSOSettings{}
 				env.fallbackStrategy.ExpectedIsMatch = false
 			},
 			identity: defaultIdentity,