keycloak_identity_provider: add hideOnLogin parameter to module

[fgruenbauer]

SUMMARY

The hideOnLoginPage parameter was moved out of the config dict and renamed to hideOnLogin in Keycloak 26. KC26 still seems to accept config/hideOnLoginPage in a request, but the response always only contains hideOnLogin.

The MR adds the parameter to the module and keeps the old parameter for compatibility.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

keycloak_identity_provider

ADDITIONAL INFORMATION

KC 26 IDP response:

{
  "alias": "IDP",
  "displayName": "IDP",
  "internalId": "cf6795b3-7e97-4a4a-82b4-767ee6a89a88",
  "providerId": "keycloak-oidc",
  "linkOnly": false,
  "hideOnLogin": false,
  "config": {
    ...
  }
}

KC 25:

{
  "alias": "keycloak-oidc",
  "displayName": "",
  "internalId": "9048593b-f534-40c5-8c49-9de4e3058580",
  "providerId": "keycloak-oidc",
  "enabled": true,
  "config": {
    "validateSignature": "false",
    "hideOnLoginPage": "true",
    "tokenUrl": "http://localhost:8999",
    ...
  }
}

See also in the docu: https://www.keycloak.org/docs-api/latest/rest-api/index.html#IdentityProviderRepresentation and https://www.keycloak.org/docs-api/25.0.6/rest-api/index.html#IdentityProviderRepresentation

[ansibullbot]

cc @eikef @laurpaum @mattock @ndclt @thomasbach-dev
click here for bot help

[felixfontein]

BTW, @fgruenbauer you might be interested in https://forum.ansible.com/t/keycloak-modules-in-community-general/41746.

[felixfontein]

Thanks for your contribution!

I'm wondering a bit whether it's not better to add a Keycloak version detection, and depending on the version moving the value to the right position. Not sure whether that's a good approach though...

[felixfontein]

I think you forgot one line from the original parameter:

Suggested change

	- If hidden, login with this provider is possible only if requested explicitly, for example using the C(kc_idp_hint)
	- If hidden, login with this provider is possible only if requested explicitly, for example using the C(kc_idp_hint)
	parameter.

[felixfontein]

Suggested change

	- keycloak_identity_provider - add ``hideOnLogin`` parameter to the module, the old parameter ``config/hideOnLoginPage`` is kept for compatibility (https://github.com/ansible-collections/community.general/pull/9983).
	- keycloak_identity_provider - add ``hideOnLogin`` parameter to the module, the old parameter ``config.hideOnLoginPage`` is kept for compatibility (https://github.com/ansible-collections/community.general/pull/9983).

[russoz]

Why is it being created at the top level and not as a suboption of config? I understand it could generate some confusion, but that would be temporary and could be mitigated with documentation.

Aren't we breaking from the pattern used in this module?

[felixfontein]

My guess is that the parameters for this module are more or less identical (up to some parameters that will be removed) to what the Keycloak API expects. In the API the value was moved from config to top-level, so this PR mirrors that.

Whether that's a good pattern - I don't know. Keeping the value in config and solving this in the code could be a liability for the future, if the value inside config is re-used for something else by Keycloak. (Maybe that even was the reason why it was moved away from config in Keycloak?)

[fgruenbauer]

Yes, in the Keycloak API the parameter was moved to the top-level, which i tried to mirror. I don't know why it was changed, though.

I thought accepting both parameters would be the most flexible solution. Users could provide the module with client configs (for example from an Keycloak config export) from both Keycloak 25 and 26 without having to edit them. Which would also allow users to update Keycloak to 26 without having to edit their existing client configs. But its also confusing to have two parameter for the same Keycloak setting.

I think, determining in code what parameter the API expects is good idea. If we accept only one parameter, i think we have to, otherwise it wouldn't work with the other Keycloak version.

It would be nice to fetch the version only once an share it between all Keycloak modules. Is it possible to set Keycloak module defaults from within a module? Otherwise it might be a good idea to allow users to provide a value, so they can make a single version request in the setup phase and use the value for all Keycloak module calls.

[felixfontein]

Is it possible to set Keycloak module defaults from within a module?

That's not possible. The only thing modules can do is set a fact, but they can't read it. (You could add an action plugin for that part, but that's "magic" with its own set of problems.)

Might be worth thinking about once the modules have been moved to a separate collection, though. I don't really want such "magic" in this collection though, it's already does way too much :)