Skip to content

Commit

Permalink
SSO: Fix SSO settings update with custom fields (#1652)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@dmihai
dmihai authored Jun 25, 2024
1 parent 32ba510 commit 49d117c
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 5 deletions.
15 changes: 13 additions & 2 deletions internal/resources/grafana/resource_sso_settings.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,14 +631,25 @@ func getSettingsFromResourceData(d *schema.ResourceData, settingsKey string) (ma
return nil, fmt.Errorf("no settings found for the provider %s", d.Get(providerKey).(string))
}

// TODO investigate why we need this
if len(settingsList) == 1 {
return settingsList[0].(map[string]any), nil
}

// sometimes the settings set contains some empty items that we want to ignore
// we are only interested in the settings that have one of the following:
// - the client_id set because the client_id is a required field for OAuth2 providers
// - the private_key or private_key_path set because those are required fields for SAML
for _, item := range settingsList {
settings := item.(map[string]any)
if settings["client_id"] != "" || settings["private_key"] != "" || settings["private_key_path"] != "" {

clientID, ok := settings["client_id"]
if ok && clientID != "" {
return settings, nil
}

privateKey, okPrivateKey := settings["private_key"]
privateKeyPath, okPrivateKeyPath := settings["private_key_path"]
if (okPrivateKey && privateKey != "") || (okPrivateKeyPath && privateKeyPath != "") {
return settings, nil
}
}
Expand Down
98 changes: 95 additions & 3 deletions internal/resources/grafana/resource_sso_settings_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,22 @@ func TestSSOSettings_basic_saml(t *testing.T) {
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.certificate_path", "devenv/docker/blocks/auth/saml-enterprise/cert.crt"),
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.private_key_path", "devenv/docker/blocks/auth/saml-enterprise/key.pem"),
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.idp_metadata_url", "https://nexus.microsoftonline-p.com/federationmetadata/saml20/federationmetadata.xml"),
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.signature_algorithm", "rsa-sha256"),
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.metadata_valid_duration", "24h"),
),
},
{
Config: testConfigForSamlProviderUpdated,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "provider_name", provider),
resource.TestCheckResourceAttr(resourceName, "saml_settings.#", "1"),
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.certificate_path", "devenv/docker/blocks/auth/saml-enterprise/cert.crt"),
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.private_key_path", "devenv/docker/blocks/auth/saml-enterprise/key.pem"),
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.idp_metadata_url", "https://nexus.microsoftonline-p.com/federationmetadata/saml20/federationmetadata.xml"),
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.signature_algorithm", "rsa-sha512"),
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.metadata_valid_duration", "48h"),
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.assertion_attribute_email", "email"),
resource.TestCheckResourceAttr(resourceName, "saml_settings.0.allow_sign_up", "true"),
),
},
{
Expand Down Expand Up @@ -157,6 +173,51 @@ func TestSSOSettings_customFields(t *testing.T) {
},
),
},
{
Config: testConfigWithCustomFieldsUpdated,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "provider_name", provider),
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.#", "1"),
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.client_id", "client_id_updated"),
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.client_secret", "client_secret"),
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.scopes", "email profile"),
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.custom.custom_field", "custom1_updated"),
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.custom.another_custom_field", "custom2_updated"),
resource.TestCheckResourceAttr(resourceName, "oauth2_settings.0.custom.one_more_custom_field", "custom4"),
// check that all custom fields are returned by the API
func(s *terraform.State) error {
resp, err := api.SsoSettings.GetProviderSettings(provider)
if err != nil {
return err
}

payload := resp.GetPayload()
settings := payload.Settings.(map[string]any)

// the API returns the settings names in camelCase
if settings["clientId"] != "client_id_updated" {
t.Fatalf("expected value for client_id is not equal to the actual value: %s", settings["clientId"])
}
if settings["scopes"] != "email profile" {
t.Fatalf("expected value for scopes is not equal to the actual value: %s", settings["scopes"])
}
if settings["customField"] != "custom1_updated" {
t.Fatalf("expected value for custom_field is not equal to the actual value: %s", settings["customField"])
}
if settings["anotherCustomField"] != "custom2_updated" {
t.Fatalf("expected value for another_custom_field is not equal to the actual value: %s", settings["anotherCustomField"])
}
if settings["oneMoreCustomField"] != "custom4" {
t.Fatalf("expected value for one_more_custom_field is not equal to the actual value: %s", settings["oneMoreCustomField"])
}
if _, ok := settings["camelCaseField"]; ok {
t.Fatalf("camelCaseField custom field is not expected to exist")
}

return nil
},
),
},
{
ResourceName: resourceName,
ImportState: true,
Expand Down Expand Up @@ -299,9 +360,24 @@ func testConfigForOAuth2Provider(provider string, prefix string) string {
const testConfigForSamlProvider = `resource "grafana_sso_settings" "saml_sso_settings" {
provider_name = "saml"
saml_settings {
certificate_path = "devenv/docker/blocks/auth/saml-enterprise/cert.crt"
private_key_path = "devenv/docker/blocks/auth/saml-enterprise/key.pem"
idp_metadata_url = "https://nexus.microsoftonline-p.com/federationmetadata/saml20/federationmetadata.xml"
certificate_path = "devenv/docker/blocks/auth/saml-enterprise/cert.crt"
private_key_path = "devenv/docker/blocks/auth/saml-enterprise/key.pem"
idp_metadata_url = "https://nexus.microsoftonline-p.com/federationmetadata/saml20/federationmetadata.xml"
signature_algorithm = "rsa-sha256"
metadata_valid_duration = "24h"
}
}`

const testConfigForSamlProviderUpdated = `resource "grafana_sso_settings" "saml_sso_settings" {
provider_name = "saml"
saml_settings {
certificate_path = "devenv/docker/blocks/auth/saml-enterprise/cert.crt"
private_key_path = "devenv/docker/blocks/auth/saml-enterprise/key.pem"
idp_metadata_url = "https://nexus.microsoftonline-p.com/federationmetadata/saml20/federationmetadata.xml"
allow_sign_up = true
signature_algorithm = "rsa-sha512"
metadata_valid_duration = "48h"
assertion_attribute_email = "email"
}
}`

Expand All @@ -318,6 +394,20 @@ const testConfigWithCustomFields = `resource "grafana_sso_settings" "sso_setting
}
}`

const testConfigWithCustomFieldsUpdated = `resource "grafana_sso_settings" "sso_settings" {
provider_name = "github"
oauth2_settings {
client_id = "client_id_updated"
client_secret = "client_secret"
scopes = "email profile"
custom = {
custom_field = "custom1_updated"
another_custom_field = "custom2_updated"
one_more_custom_field = "custom4"
}
}
}`

const testConfigWithEmptySettings = `resource "grafana_sso_settings" "sso_settings" {
provider_name = "okta"
oauth2_settings {
Expand Down Expand Up @@ -449,6 +539,8 @@ var testConfigsWithValidationErrors = []string{
saml_settings {
certificate = "this-is-a-valid-certificate"
certificate_path = "/valid/certificate/path"
private_key = "this-is-a-valid-private-key"
idp_metadata_path = "/path/to/metadata"
}
}`,
// missing idp_metadata for saml
Expand Down

0 comments on commit 49d117c

Please sign in to comment.