Skip to content

Commit 49d117c

Browse files
dmihaidmihai
authored
SSO: Fix SSO settings update with custom fields (#1652)
* fix settings retrieval when there are multiple settings * fix lint error * fix saml input for validation tests * return settings if there is just one in the set
1 parent 32ba510 commit 49d117c

File tree

2 files changed

+108
-5
lines changed

2 files changed

+108
-5
lines changed

‎internal/resources/grafana/resource_sso_settings.go‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -631,14 +631,25 @@ func getSettingsFromResourceData(d *schema.ResourceData, settingsKey string) (ma
631631
return nil, fmt.Errorf("no settings found for the provider %s", d.Get(providerKey).(string))
632632
}
633633

634-
// TODO investigate why we need this
634+
if len(settingsList) == 1 {
635+
return settingsList[0].(map[string]any), nil
636+
}
637+
635638
// sometimes the settings set contains some empty items that we want to ignore
636639
// we are only interested in the settings that have one of the following:
637640
// - the client_id set because the client_id is a required field for OAuth2 providers
638641
// - the private_key or private_key_path set because those are required fields for SAML
639642
for _, item := range settingsList {
640643
settings := item.(map[string]any)
641-
if settings["client_id"] != "" || settings["private_key"] != "" || settings["private_key_path"] != "" {
644+
645+
clientID, ok := settings["client_id"]
646+
if ok && clientID != "" {
647+
return settings, nil
648+
}
649+
650+
privateKey, okPrivateKey := settings["private_key"]
651+
privateKeyPath, okPrivateKeyPath := settings["private_key_path"]
652+
if (okPrivateKey && privateKey != "") || (okPrivateKeyPath && privateKeyPath != "") {
642653
return settings, nil
643654
}
644655
}

‎internal/resources/grafana/resource_sso_settings_test.go‎

Lines changed: 95 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,22 @@ func TestSSOSettings_basic_saml(t *testing.T) {
8989
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.certificate_path", "devenv/docker/blocks/auth/saml-enterprise/cert.crt"),
9090
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.private_key_path", "devenv/docker/blocks/auth/saml-enterprise/key.pem"),
9191
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.idp_metadata_url", "https://nexus.microsoftonline-p.com/federationmetadata/saml20/federationmetadata.xml"),
92+
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.signature_algorithm", "rsa-sha256"),
93+
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.metadata_valid_duration", "24h"),
94+
),
95+
},
96+
{
97+
Config: testConfigForSamlProviderUpdated,
98+
Check: resource.ComposeTestCheckFunc(
99+
resource.TestCheckResourceAttr(resourceName, "provider_name", provider),
100+
resource.TestCheckResourceAttr(resourceName, "saml_settings.#", "1"),
101+
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.certificate_path", "devenv/docker/blocks/auth/saml-enterprise/cert.crt"),
102+
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.private_key_path", "devenv/docker/blocks/auth/saml-enterprise/key.pem"),
103+
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.idp_metadata_url", "https://nexus.microsoftonline-p.com/federationmetadata/saml20/federationmetadata.xml"),
104+
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.signature_algorithm", "rsa-sha512"),
105+
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.metadata_valid_duration", "48h"),
106+
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.assertion_attribute_email", "email"),
107+
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.allow_sign_up", "true"),
92108
),
93109
},
94110
{
@@ -157,6 +173,51 @@ func TestSSOSettings_customFields(t *testing.T) {
157173
},
158174
),
159175
},
176+
{
177+
Config: testConfigWithCustomFieldsUpdated,
178+
Check: resource.ComposeTestCheckFunc(
179+
resource.TestCheckResourceAttr(resourceName, "provider_name", provider),
180+
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.#", "1"),
181+
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.client_id", "client_id_updated"),
182+
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.client_secret", "client_secret"),
183+
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.scopes", "email profile"),
184+
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.custom.custom_field", "custom1_updated"),
185+
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.custom.another_custom_field", "custom2_updated"),
186+
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.custom.one_more_custom_field", "custom4"),
187+
// check that all custom fields are returned by the API
188+
func(s *terraform.State) error {
189+
resp, err := api.SsoSettings.GetProviderSettings(provider)
190+
if err != nil {
191+
return err
192+
}
193+
194+
payload := resp.GetPayload()
195+
settings := payload.Settings.(map[string]any)
196+
197+
// the API returns the settings names in camelCase
198+
if settings["clientId"] != "client_id_updated" {
199+
t.Fatalf("expected value for client_id is not equal to the actual value: %s", settings["clientId"])
200+
}
201+
if settings["scopes"] != "email profile" {
202+
t.Fatalf("expected value for scopes is not equal to the actual value: %s", settings["scopes"])
203+
}
204+
if settings["customField"] != "custom1_updated" {
205+
t.Fatalf("expected value for custom_field is not equal to the actual value: %s", settings["customField"])
206+
}
207+
if settings["anotherCustomField"] != "custom2_updated" {
208+
t.Fatalf("expected value for another_custom_field is not equal to the actual value: %s", settings["anotherCustomField"])
209+
}
210+
if settings["oneMoreCustomField"] != "custom4" {
211+
t.Fatalf("expected value for one_more_custom_field is not equal to the actual value: %s", settings["oneMoreCustomField"])
212+
}
213+
if _, ok := settings["camelCaseField"]; ok {
214+
t.Fatalf("camelCaseField custom field is not expected to exist")
215+
}
216+
217+
return nil
218+
},
219+
),
220+
},
160221
{
161222
ResourceName: resourceName,
162223
ImportState: true,
@@ -299,9 +360,24 @@ func testConfigForOAuth2Provider(provider string, prefix string) string {
299360
const testConfigForSamlProvider = `resource "grafana_sso_settings" "saml_sso_settings" {
300361
provider_name = "saml"
301362
saml_settings {
302-
certificate_path = "devenv/docker/blocks/auth/saml-enterprise/cert.crt"
303-
private_key_path = "devenv/docker/blocks/auth/saml-enterprise/key.pem"
304-
idp_metadata_url = "https://nexus.microsoftonline-p.com/federationmetadata/saml20/federationmetadata.xml"
363+
certificate_path = "devenv/docker/blocks/auth/saml-enterprise/cert.crt"
364+
private_key_path = "devenv/docker/blocks/auth/saml-enterprise/key.pem"
365+
idp_metadata_url = "https://nexus.microsoftonline-p.com/federationmetadata/saml20/federationmetadata.xml"
366+
signature_algorithm = "rsa-sha256"
367+
metadata_valid_duration = "24h"
368+
}
369+
}`
370+
371+
const testConfigForSamlProviderUpdated = `resource "grafana_sso_settings" "saml_sso_settings" {
372+
provider_name = "saml"
373+
saml_settings {
374+
certificate_path = "devenv/docker/blocks/auth/saml-enterprise/cert.crt"
375+
private_key_path = "devenv/docker/blocks/auth/saml-enterprise/key.pem"
376+
idp_metadata_url = "https://nexus.microsoftonline-p.com/federationmetadata/saml20/federationmetadata.xml"
377+
allow_sign_up = true
378+
signature_algorithm = "rsa-sha512"
379+
metadata_valid_duration = "48h"
380+
assertion_attribute_email = "email"
305381
}
306382
}`
307383

@@ -318,6 +394,20 @@ const testConfigWithCustomFields = `resource "grafana_sso_settings" "sso_setting
318394
}
319395
}`
320396

397+
const testConfigWithCustomFieldsUpdated = `resource "grafana_sso_settings" "sso_settings" {
398+
provider_name = "github"
399+
oauth2_settings {
400+
client_id = "client_id_updated"
401+
client_secret = "client_secret"
402+
scopes = "email profile"
403+
custom = {
404+
custom_field = "custom1_updated"
405+
another_custom_field = "custom2_updated"
406+
one_more_custom_field = "custom4"
407+
}
408+
}
409+
}`
410+
321411
const testConfigWithEmptySettings = `resource "grafana_sso_settings" "sso_settings" {
322412
provider_name = "okta"
323413
oauth2_settings {
@@ -449,6 +539,8 @@ var testConfigsWithValidationErrors = []string{
449539
saml_settings {
450540
certificate = "this-is-a-valid-certificate"
451541
certificate_path = "/valid/certificate/path"
542+
private_key = "this-is-a-valid-private-key"
543+
idp_metadata_path = "/path/to/metadata"
452544
}
453545
}`,
454546
// missing idp_metadata for saml

0 commit comments

Comments
 (0)