Add root_url support to OpenID client

[languitar]

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:

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"

Then directly clicking "save" results in:

However:

results in:

As you can see, three additional URLs in the bottom are pre-filled.

This is also reflected in the output of the REST API:

  {
    "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 API. 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

[mrparkers]

LGTM - thanks for the PR!

[languitar]

Thanks for your help!