SSO: fix mergeSettings() in case the DB contains empty URLs (#84290)

* fix mergeSettings() in case the db contains empty strings

* use correct github urls in test

* overwrite only urls

* update comment for mergeSettings()

pkg/services/ssosettings/ssosettingsimpl/service.go

@@ -433,6 +433,7 @@ func removeSecrets(settings map[string]any) map[string]any {
 
 // mergeSettings merges two maps in a way that the values from the first map are preserved
 // and the values from the second map are added only if they don't exist in the first map
+// or if they contain empty URLs.
 func mergeSettings(storedSettings, systemSettings map[string]any) map[string]any {
 	settings := make(map[string]any)
 
@@ -443,6 +444,12 @@ func mergeSettings(storedSettings, systemSettings map[string]any) map[string]any
 	for k, v := range systemSettings {
 		if _, ok := settings[k]; !ok {
 			settings[k] = v
+		} else if isURL(k) && isEmptyString(settings[k]) {
+			// Overwrite all URL settings from the DB containing an empty string with their value
+			// from the system settings. This fixes an issue with empty auth_url, api_url and token_url
+			// from the DB not being replaced with their values defined in the system settings for
+			// the Google provider.
+			settings[k] = v
 		}
 	}
 
@@ -486,6 +493,15 @@ func isSecret(fieldName string) bool {
 	return false
 }
 
+func isURL(fieldName string) bool {
+	return strings.HasSuffix(fieldName, "_url")
+}
+
+func isEmptyString(val any) bool {
+	_, ok := val.(string)
+	return ok && val == ""
+}
+
 func isNewSecretValue(value string) bool {
 	return value != setting.RedactedPassword
 }

pkg/services/ssosettings/ssosettingsimpl/service_test.go

@@ -185,6 +185,42 @@ func TestService_GetForProvider(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "correctly merge the DB and system settings",
+			setup: func(env testEnv) {
+				env.store.ExpectedSSOSetting = &models.SSOSettings{
+					Provider: "github",
+					Settings: map[string]any{
+						"enabled":  true,
+						"auth_url": "",
+						"api_url":  "https://overwritten-api.com/user",
+						"team_ids": "",
+					},
+					Source: models.DB,
+				}
+				env.fallbackStrategy.ExpectedIsMatch = true
+				env.fallbackStrategy.ExpectedConfigs = map[string]map[string]any{
+					"github": {
+						"auth_url":  "https://github.com/login/oauth/authorize",
+						"token_url": "https://github.com/login/oauth/access_token",
+						"api_url":   "https://api.github.com/user",
+						"team_ids":  "10,11,12",
+					},
+				}
+			},
+			want: &models.SSOSettings{
+				Provider: "github",
+				Settings: map[string]any{
+					"enabled":   true,
+					"auth_url":  "https://github.com/login/oauth/authorize",
+					"token_url": "https://github.com/login/oauth/access_token",
+					"api_url":   "https://overwritten-api.com/user",
+					"team_ids":  "",
+				},
+				Source: models.DB,
+			},
+			wantErr: false,
+		},
 	}
 
 	for _, tc := range testCases {