Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions pkg/services/ssosettings/ssosettingsimpl/service.go
Expand Up @@ -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)

Expand All @@ -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
}
}

Expand Down Expand Up @@ -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
}
36 changes: 36 additions & 0 deletions pkg/services/ssosettings/ssosettingsimpl/service_test.go
Expand Up @@ -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 {
Expand Down