Repository: mrparkers/terraform-provider-keycloak
Pull request #248 (2020-03-09 16:56:14)

Add root_url support to OpenID client
I am trying to add the requested root_url support (#236) to the OpenID client. However,  at least locally, tests fail after my changes and I don't understand the reason for that. Maybe someone has a helping hand on what is missing here?

languitar: Alright, once root_url is set, web_origins defaults to root_url and valid_redirect_uris to [root_url/*]. Should these values be implied by the provider itself or should an error be reported that things have to be configured manually?
languitar: I'm stuck here. If I declare `root_url` as `omitempty`, I can conditionally create clients with a configured root URL or, if not specified, provide the existing behavior. However, `omitempty` prevents me from removing the existing root URL, because if the URL was previously set, the a change that should remove this field doesn't include it anymore and hence results in a noop regarding this option. That's the reason why the update test fails. With the alternative of always setting `root_url` (if not specified to empty strings) the automatic generation of defaults for web_origins, valid_redirect_uris etc. is always triggered and none of the existing configurations would work anymore.
mrparkers: Hey @languitar, I'm trying to reproduce the issue you're having but I'm having some trouble with it.  I'm running Keycloak 8.0.1 locally, and when I set a root url on a client, it doesn't appear to modify any other url attributes for the client.

Could you walk me through the steps you're taking to reproduce this?
languitar: Thanks for the quick feedback. Here's the procedure to demonstrate this just using the GUI:

```sh
docker run --rm -e KEYCLOAK_USER=keycloak -e KEYCLOAK_PASSWORD=password -p 8080:8080 jboss/keycloak:8.0.1 "-b" "0.0.0.0" "-Dkeycloak.profile.feature.upload_scripts=enabled"
```

![image](https://user-images.githubusercontent.com/1336287/76331902-fd06ab80-62ef-11ea-9751-3977e7c0f9b5.png)

Then directly clicking "save" results in:

![image](https://user-images.githubusercontent.com/1336287/76331933-0859d700-62f0-11ea-9a68-b1092ffe7ba9.png)

However:

![image](https://user-images.githubusercontent.com/1336287/76332022-24f60f00-62f0-11ea-9d17-576cc7bf5626.png)

results in:

![image](https://user-images.githubusercontent.com/1336287/76332049-2fb0a400-62f0-11ea-9fe7-7c935c398a7f.png)

As you can see, three additional URLs in the bottom are pre-filled.

This is also reflected in the output of the REST API:

```json
  {
    "id": "5279791e-4c41-42d0-8e8d-ff139d87208c",
    "clientId": "withroot",
    "rootUrl": "http://root",
    "adminUrl": "http://root",
    "surrogateAuthRequired": false,
    "enabled": true,
    "clientAuthenticatorType": "client-secret",
    "redirectUris": [
      "http://root/*"
    ],
    "webOrigins": [
      "http://root"
    ],
    "notBefore": 0,
    "bearerOnly": false,
    "consentRequired": false,
    "standardFlowEnabled": true,
    "implicitFlowEnabled": false,
    "directAccessGrantsEnabled": true,
    "serviceAccountsEnabled": false,
    "publicClient": true,
    "frontchannelLogout": false,
    "protocol": "openid-connect",
    "attributes": {},
    "authenticationFlowBindingOverrides": {},
    "fullScopeAllowed": true,
    "nodeReRegistrationTimeout": -1,
    "defaultClientScopes": [
      "web-origins",
      "role_list",
      "roles",
      "profile",
      "email"
    ],
    "optionalClientScopes": [
      "address",
      "phone",
      "offline_access",
      "microprofile-jwt"
    ],
    "access": {
      "view": true,
      "configure": true,
      "manage": true
    }
  }
```
mrparkers: Ah, I see.  So it looks like this is only happening when a client is created, not updated.  I assume that this happens to help the user understand what the root url is trying to do.

This is a tricky one, but I think we can find a way around it.  If I understand correctly, the problem you're running into is that when you use `omitempty` for `rootUrl`, there is no way to unset it once it's been set.  But when you don't use `omitempty`, creating a client without a `root_url` attribute will cause the `valid_redirect_urls` and `web_origins` to be set to a default, even when they are not specified.

One solution to this problem is to update the `valid_redirect_urls` and `web_origins` attributes to be `Computed`.  This will allow the tests to pass, as the provider will expect them to potentially have values even when they aren't explicitly set.  I'm not sure I like this solution because I believe the default value for these attributes when `root_url` is missing is `["*/*"]`, which I think is a dangerous default.

I think the best solution to this problem is to use a string pointer for the `rootUrl` property of the `OpenidClient` struct.  This will allow us to be in control of when the `rootUrl` json value is actually omitted.  When the pointer is nil, the value will be omitted, and when the pointer is not nil (even if it points to an empty string), the json value will be kept.

The trick to this is to use `rootUrlFromData, ok := data.GetOkExists("root_url")` to get the value from the configuration.  If the configuration for a `keycloak_openid_client` resource has ever specified a `root_url`, (even if `root_url` is not specified now) then `ok` will be true.

So, when you create a `keycloak_openid_client` resource with no `root_url` attribute, then `ok` will be false, and you can set `openidClient.rootUrl = nil`, so that json value won't ever make it to Keycloak.  If the `root_url` attribute is specified, then `ok` will be true, so you can set `openidClient.rootUrl = rootUrlFromData`, which will set the root url in Keycloak.  Then, if the `root_url` attribute is removed, `ok` will still be true, since it was previously set at one point, so you can set `openidClient.rootUrl = rootUrlFromData` (`rootUrlFromData` will be an empty string) which is a non-nil pointer, so Keycloak will receive the empty string and the root url will be removed.

Sorry for the long-winded explanation, I hope that makes sense.  If you're still having trouble with this, I can try to make a commit against your branch to see if I can fix it.
languitar: Thanks for the long and detailed explanations. However, I am still struggling to find a working solution (with my limited knowledge of Go and Keycloak). I have pushed what I have tried so far with pointers. But now a lot of tests end up in open changes on the various other URLs again:

```
          valid_redirect_uris.#:                    "1" => "0"
          valid_redirect_uris.0:                    "/*" => ""
          web_origins.#:                            "1" => "0"
          web_origins.0:                            "/*" => ""
```

If you have some time to take a look at how to solve this, that would be great.
mrparkers: Thanks for giving that a shot.  I went ahead and made some changes and pushed them to your branch.  Would you mind building these changes and verifying that they work for your use case?
languitar: Thanks for your help!

I think one issue remains: even though keycloak initially creates an admin_url when using root_url, it is not required to have something in admin_url. Not specifying admin_url as well as using an empty string now results in "admin_url is required when root_url is given". We could probably live with that by specifying some dummy URL, but in theory it should be possible to have an empty field here.
mrparkers: I think we can mark those url attributes as `Computed` and `Optional`, so if the user doesn't specify anything for these attributes, they'll still be persisted to state as whatever Keycloak sets them to.  Feel free to give that a shot.
languitar: Sorry for the long delay. I don't think that would help either, because what I would want to create is a client that only has a root url but no admin url. This is simply not possible in one pass with the keycloak admin URL. So I think the current behavior is probably the best thing we can do with this strange default behavior that is implemented in keycloak. So I'd be fine to leave the PR as it is.
mrparkers: That works for me!  Could you update your branch to resolve the merge conflicts?
languitar: Alright, I have rebased this and squashed things into a single commit

Patch:
diff --git a/keycloak/openid_client.go b/keycloak/openid_client.go
index fd9e922..92b4a4f 100644
--- a/keycloak/openid_client.go
+++ b/keycloak/openid_client.go
@@ -45,6 +45,7 @@ type OpenidClient struct {
 	WebOrigins                         []string                                 `json:"webOrigins"`
 	AdminUrl                           string                                   `json:"adminUrl"`
 	BaseUrl                            string                                   `json:"baseUrl"`
+	RootUrl                            *string                                  `json:"rootUrl,omitempty"`
 	FullScopeAllowed                   bool                                     `json:"fullScopeAllowed"`
 	Attributes                         OpenidClientAttributes                   `json:"attributes"`
 	AuthorizationSettings              *OpenidClientAuthorizationSettings       `json:"authorizationSettings,omitempty"`
diff --git a/provider/data_source_keycloak_openid_client.go b/provider/data_source_keycloak_openid_client.go
index eb5caea..a9e8243 100644
--- a/provider/data_source_keycloak_openid_client.go
+++ b/provider/data_source_keycloak_openid_client.go
@@ -71,6 +71,10 @@ func dataSourceKeycloakOpenidClient() *schema.Resource {
 				Type:     schema.TypeBool,
 				Computed: true,
 			},
+			"root_url": {
+				Type:     schema.TypeString,
+				Computed: true,
+			},
 			"resource_server_id": {
 				Type:     schema.TypeString,
 				Computed: true,
diff --git a/provider/resource_keycloak_openid_client.go b/provider/resource_keycloak_openid_client.go
index ac3b3b3..22aed5a 100644
--- a/provider/resource_keycloak_openid_client.go
+++ b/provider/resource_keycloak_openid_client.go
@@ -94,6 +94,10 @@ func resourceKeycloakOpenidClient() *schema.Resource {
 				Type:     schema.TypeString,
 				Optional: true,
 			},
+			"root_url": {
+				Type:     schema.TypeString,
+				Optional: true,
+			},
 			"service_accounts_enabled": {
 				Type:     schema.TypeBool,
 				Optional: true,
@@ -180,18 +184,37 @@ func getOpenidClientFromData(data *schema.ResourceData) (*keycloak.OpenidClient,
 	validRedirectUris := make([]string, 0)
 	webOrigins := make([]string, 0)
 
-	if v, ok := data.GetOk("valid_redirect_uris"); ok {
-		for _, validRedirectUri := range v.(*schema.Set).List() {
+	rootUrlData, rootUrlOk := data.GetOkExists("root_url")
+	validRedirectUrisData, validRedirectUrisOk := data.GetOk("valid_redirect_uris")
+	webOriginsData, webOriginsOk := data.GetOk("web_origins")
+
+	rootUrlString := rootUrlData.(string)
+
+	if validRedirectUrisOk {
+		for _, validRedirectUri := range validRedirectUrisData.(*schema.Set).List() {
 			validRedirectUris = append(validRedirectUris, validRedirectUri.(string))
 		}
 	}
 
-	if v, ok := data.GetOk("web_origins"); ok {
-		for _, webOrigin := range v.(*schema.Set).List() {
+	if webOriginsOk {
+		for _, webOrigin := range webOriginsData.(*schema.Set).List() {
 			webOrigins = append(webOrigins, webOrigin.(string))
 		}
 	}
 
+	// Keycloak uses the root URL for web origins if not specified otherwise
+	if rootUrlOk && rootUrlString != "" {
+		if !validRedirectUrisOk {
+			return nil, errors.New("valid_redirect_uris is required when root_url is given1")
+		}
+		if !webOriginsOk {
+			return nil, errors.New("web_origins is required when root_url is given")
+		}
+		if _, adminOk := data.GetOk("admin_url"); !adminOk {
+			return nil, errors.New("admin_url is required when root_url is given")
+		}
+	}
+
 	openidClient := &keycloak.OpenidClient{
 		Id:                        data.Id(),
 		ClientId:                  data.Get("client_id").(string),
@@ -217,6 +240,10 @@ func getOpenidClientFromData(data *schema.ResourceData) (*keycloak.OpenidClient,
 		ConsentRequired:   data.Get("consent_required").(bool),
 	}
 
+	if rootUrlOk {
+		openidClient.RootUrl = &rootUrlString
+	}
+
 	if !openidClient.ImplicitFlowEnabled && !openidClient.StandardFlowEnabled {
 		if _, ok := data.GetOk("valid_redirect_uris"); ok {
 			return nil, errors.New("valid_redirect_uris cannot be set when standard or implicit flow is not enabled")
@@ -285,6 +312,7 @@ func setOpenidClientData(keycloakClient *keycloak.KeycloakClient, data *schema.R
 	data.Set("web_origins", client.WebOrigins)
 	data.Set("admin_url", client.AdminUrl)
 	data.Set("base_url", client.BaseUrl)
+	data.Set("root_url", &client.RootUrl)
 	data.Set("authorization_services_enabled", client.AuthorizationServicesEnabled)
 	data.Set("full_scope_allowed", client.FullScopeAllowed)
 	data.Set("consent_required", client.ConsentRequired)


Test patch:
diff --git a/provider/resource_keycloak_openid_client_test.go b/provider/resource_keycloak_openid_client_test.go
index f9c3f47..755c343 100644
--- a/provider/resource_keycloak_openid_client_test.go
+++ b/provider/resource_keycloak_openid_client_test.go
@@ -156,6 +156,24 @@ func TestAccKeycloakOpenidClient_baseUrl(t *testing.T) {
 	})
 }
 
+func TestAccKeycloakOpenidClient_rootUrl(t *testing.T) {
+	realmName := "terraform-" + acctest.RandString(10)
+	clientId := "terraform-" + acctest.RandString(10)
+	rootUrl := "https://www.example.com"
+
+	resource.Test(t, resource.TestCase{
+		Providers:    testAccProviders,
+		PreCheck:     func() { testAccPreCheck(t) },
+		CheckDestroy: testAccCheckKeycloakOpenidClientDestroy(),
+		Steps: []resource.TestStep{
+			{
+				Config: testKeycloakOpenidClient_rootUrl(realmName, clientId, rootUrl),
+				Check:  testAccCheckKeycloakOpenidClientRootUrl("keycloak_openid_client.client", rootUrl),
+			},
+		},
+	})
+}
+
 func TestAccKeycloakOpenidClient_updateInPlace(t *testing.T) {
 	realm := "terraform-" + acctest.RandString(10)
 	clientId := "terraform-" + acctest.RandString(10)
@@ -169,6 +187,7 @@ func TestAccKeycloakOpenidClient_updateInPlace(t *testing.T) {
 		implicitFlowEnabled = !standardFlowEnabled
 	}
 
+	rootUrlBefore := acctest.RandString(20)
 	openidClientBefore := &keycloak.OpenidClient{
 		RealmId:                   realm,
 		ClientId:                  clientId,
@@ -184,10 +203,12 @@ func TestAccKeycloakOpenidClient_updateInPlace(t *testing.T) {
 		WebOrigins:                []string{acctest.RandString(10), acctest.RandString(10), acctest.RandString(10)},
 		AdminUrl:                  acctest.RandString(20),
 		BaseUrl:                   acctest.RandString(20),
+		RootUrl:                   &rootUrlBefore,
 	}
 
 	standardFlowEnabled, implicitFlowEnabled = implicitFlowEnabled, standardFlowEnabled
 
+	rootUrlAfter := acctest.RandString(20)
 	openidClientAfter := &keycloak.OpenidClient{
 		RealmId:                   realm,
 		ClientId:                  clientId,
@@ -203,6 +224,7 @@ func TestAccKeycloakOpenidClient_updateInPlace(t *testing.T) {
 		WebOrigins:                []string{acctest.RandString(10), acctest.RandString(10), acctest.RandString(10), acctest.RandString(10), acctest.RandString(10)},
 		AdminUrl:                  acctest.RandString(20),
 		BaseUrl:                   acctest.RandString(20),
+		RootUrl:                   &rootUrlAfter,
 	}
 
 	resource.Test(t, resource.TestCase{
@@ -539,6 +561,21 @@ func testAccCheckKeycloakOpenidClientBaseUrl(resourceName string, baseUrl string
 	}
 }
 
+func testAccCheckKeycloakOpenidClientRootUrl(resourceName string, rootUrl string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		client, err := getOpenidClientFromState(s, resourceName)
+		if err != nil {
+			return err
+		}
+
+		if *client.RootUrl != rootUrl {
+			return fmt.Errorf("expected openid client to have rootUrl set to %s, but got %s", rootUrl, *client.RootUrl)
+		}
+
+		return nil
+	}
+}
+
 func testAccCheckKeycloakOpenidClientBelongsToRealm(resourceName, realm string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		client, err := getOpenidClientFromState(s, resourceName)
@@ -777,6 +814,25 @@ resource "keycloak_openid_client" "client" {
 	`, realm, clientId, baseUrl)
 }
 
+func testKeycloakOpenidClient_rootUrl(realm, clientId, rootUrl string) string {
+	return fmt.Sprintf(`
+resource "keycloak_realm" "realm" {
+	realm = "%s"
+}
+
+resource "keycloak_openid_client" "client" {
+	client_id             = "%s"
+	realm_id			  = "${keycloak_realm.realm.id}"
+	root_url			  = "%s"
+	valid_redirect_uris   = ["http://example.com"]
+	web_origins			  = ["http://example.com"]
+	admin_url			  = "http://example.com"
+	access_type           = "CONFIDENTIAL"
+	standard_flow_enabled = true
+}
+	`, realm, clientId, rootUrl)
+}
+
 func testKeycloakOpenidClient_pkceChallengeMethod(realm, clientId, pkceChallengeMethod string) string {
 
 	return fmt.Sprintf(`
@@ -901,8 +957,9 @@ resource "keycloak_openid_client" "client" {
 	web_origins                  = %s
 	admin_url					 = "%s"
 	base_url                     = "%s"
+	root_url                     = "%s"
 }
-	`, openidClient.RealmId, openidClient.ClientId, openidClient.Name, openidClient.Enabled, openidClient.Description, openidClient.ClientSecret, openidClient.StandardFlowEnabled, openidClient.ImplicitFlowEnabled, openidClient.DirectAccessGrantsEnabled, openidClient.ServiceAccountsEnabled, arrayOfStringsForTerraformResource(openidClient.ValidRedirectUris), arrayOfStringsForTerraformResource(openidClient.WebOrigins), openidClient.AdminUrl, openidClient.BaseUrl)
+	`, openidClient.RealmId, openidClient.ClientId, openidClient.Name, openidClient.Enabled, openidClient.Description, openidClient.ClientSecret, openidClient.StandardFlowEnabled, openidClient.ImplicitFlowEnabled, openidClient.DirectAccessGrantsEnabled, openidClient.ServiceAccountsEnabled, arrayOfStringsForTerraformResource(openidClient.ValidRedirectUris), arrayOfStringsForTerraformResource(openidClient.WebOrigins), openidClient.AdminUrl, openidClient.BaseUrl, *openidClient.RootUrl)
 }
 
 func testKeycloakOpenidClient_secret(realm, clientId, clientSecret string) string {